Skip to content

solaraviz kwargs related bugfix #2463

New issue

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

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

Already on GitHub? # to your account

Merged
merged 6 commits into from
Nov 6, 2024
Merged

solaraviz kwargs related bugfix #2463

merged 6 commits into from
Nov 6, 2024

Conversation

quaquel
Copy link
Member

@quaquel quaquel commented Nov 6, 2024

If MyModel .__init__() uses **kwargs to allow additional optional model configuration this errors because of the parameter check introduced in #2454. This PR addresses this by excluding kwargs explicitly in _check_model_params.

In passing, it also removes a user warning related to the conditional use of hooks introduced in #2460.

This closes #2462.

rewrite to deal with solara conditional use
@quaquel quaquel requested a review from Corvince November 6, 2024 19:07
@quaquel quaquel added the bug Release notes label label Nov 6, 2024

This comment was marked as off-topic.

@quaquel
Copy link
Member Author

quaquel commented Nov 6, 2024

Thinking about this a bit more, I am wondering whether we should not also change

    for name in model_params:
        if name not in model_parameters:
            raise ValueError(f"Invalid model parameter: {name}")

to

    for name in model_params:
        if (name not in model_parameters
           and "kwargs" not in model_parameters
        ):
            raise ValueError(f"Invalid model parameter: {name}")

That is, if you have a user settable parameter in model_params but are using **kwargs in the signature, the user settable parameter will work fine.

Copy link
Contributor

@Corvince Corvince left a comment

Choose a reason for hiding this comment

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

Nice quick fix and agree with your comment - when kwargs are used we should not raise an Error.

Would be good to also add both cases to the unit tests

@quaquel quaquel merged commit 0c1ccd6 into projectmesa:main Nov 6, 2024
11 of 12 checks passed
@quaquel quaquel deleted the kwargs branch November 7, 2024 09:40
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PR #2454 prevents use of **kwargs in model initialization
2 participants