Fix BaseMetaObject.set_params to correctly call reset method
Perfect! Here’s how you can fill out the PR using their template, keeping it clean, detailed, and aligned with the contribution guidelines:
Reference Issues/PRs
Issue #412
What does this implement/fix? Explain your changes.
This PR adds a unit test to verify that BaseMetaObject.set_params correctly calls the reset() method when parameters are updated.
Previously, the test failed due to a missing steps attribute, which is required by BaseMetaObject during parameter handling. This issue is resolved by explicitly defining steps=[] in the test subclass used for validation.
Does your contribution introduce a new dependency? If yes, which one?
No
What should a reviewer concentrate their feedback on?
- [x] Correctness of test logic validating the
reset()method call - [x] Proper subclass usage of
BaseMetaObjectin the test - [x] Whether the test accurately reflects realistic usage patterns
Any other comments?
All tests pass locally (1572 passed, 1 warning). This test improves coverage and ensures robustness of metaobject behavior during param updates.
PR checklist
For all contributions
- [x] I've reviewed the project documentation on [contributing](https://skbase.readthedocs.io/en/latest/contribute.html)
- [x] The PR title starts with [BUG] to indicate this is a bug fix
For code contributions
- [x] Unit tests have been added covering code functionality
- [x] Appropriate docstrings have been added
- [x] New public functionality has been added to the API Reference (not applicable here)
Thanks for working on this @Ankush1oo8.
In addition to comments by @fkiraly, I would like the test to set another variable in __init__ that is derived from a. E.g. self.b_ = 2*a, or equivalently it could set a dynamic tag that depends ona. The test would then assert that the derived param is updated to the expected value when a is set
Plus, _set_params calls super().set_params which has a reset. Why does this not have the intended effect?
super().set_params has a short circuit if there are no more params to set. In this case reset is not called. Hence the need to call reset explicitly here.
@wilsbj and @fkiraly I think so I havee made the changes you suggested
Thanks @Ankush1oo8.
Does the test still pass if you remove the hardcoded definition for reset?
The expectation is that the definition of reset on BaseObject will already perform this update for us, as long as reset is called at the right time.