Skip to content

Conversation

@Ankush1oo8
Copy link

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?

  • Correctness of test logic validating the reset() method call
  • Proper subclass usage of BaseMetaObject in the test
  • 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
For code contributions
  • Unit tests have been added covering code functionality
  • Appropriate docstrings have been added
  • New public functionality has been added to the API Reference (not applicable here)

Copy link
Contributor

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, good start!

I think this does not address the issue though - reset needs to be called after the parameters have been set.

Plus, _set_params calls super().set_params which has a reset. Why does this not have the intended effect?

Plus, some of the matching logic in BaseObject is missing. I would recommend to do a refactor where

The test is not appropriate though, since it overwrites reset.

Instead, I would write a parameter to obj and check whether it gets removed.

@wilsbj
Copy link

wilsbj commented Apr 7, 2025

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.

@Ankush1oo8
Copy link
Author

@wilsbj and @fkiraly I think so I havee made the changes you suggested

@Ankush1oo8 Ankush1oo8 requested a review from fkiraly April 8, 2025 16:25
@wilsbj
Copy link

wilsbj commented Apr 8, 2025

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.

class ResetCheckMetaObject(BaseMetaObject):
def __init__(self, a=1, steps=None):
self.a = a
self.steps = steps if steps is not None else []
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not compliant with the API - self params should never be changed.

There should also be steps_ etc, and we should also do get_params in the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants