Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow components as a positional argument again #2488

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

Corvince
Copy link
Contributor

Followup on #2485

I missed this in the code review. I think "components" should also be allowed as a positional argument. Thats at least how I usually use it when playing around. My mental model is that "model" and "components" are required arguments, which should always be provided and the other arguments can be provided, but in no specific order. The counter argument is that components also have a default value ("default"), so my mental model is not fully correct since its not strictly required. @quaquel whats your stance on this?

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔵 -0.3% [-1.2%, +0.5%] 🔵 +0.1% [-0.4%, +0.5%]
BoltzmannWealth large 🔵 +0.6% [-0.4%, +1.4%] 🔵 +0.3% [-0.0%, +0.5%]
Schelling small 🔵 +0.0% [-0.5%, +0.5%] 🔵 +0.7% [+0.4%, +1.1%]
Schelling large 🔵 -0.6% [-2.1%, +0.8%] 🔵 -0.4% [-0.9%, +0.1%]
WolfSheep small 🔵 +0.5% [-0.3%, +1.3%] 🔵 -0.8% [-1.3%, -0.3%]
WolfSheep large 🔵 -0.5% [-1.8%, +0.5%] 🔵 -0.8% [-1.5%, +0.0%]
BoidFlockers small 🟢 -6.4% [-7.5%, -5.2%] 🟢 -4.9% [-5.9%, -4.0%]
BoidFlockers large 🟢 -6.3% [-8.1%, -4.6%] 🔵 -1.6% [-2.4%, -1.1%]

@EwoutH EwoutH requested a review from quaquel November 11, 2024 13:26
Copy link
Member

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'm good with it, let's see what Jan thinks.

@quaquel
Copy link
Member

quaquel commented Nov 11, 2024

I did not think about it much when I added *. I checked a few, but not all, examples to see if components was used as a keyword argument and kept it that way. It makes good sense to me to change this. There is no point to a UI without model output components.

@Corvince Corvince merged commit e1d886e into main Nov 11, 2024
11 of 12 checks passed
@EwoutH EwoutH added the bug Release notes label label Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants