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

Avoid RecursionError when using Model.set methods inside Model.read methods #473

Merged
merged 3 commits into from
Aug 9, 2023

Conversation

DirkEilander
Copy link
Contributor

@DirkEilander DirkEilander commented Aug 8, 2023

Issue addressed

Fixes #472

Explanation

I moved the initialization of the model component to the property definition.
When called the first time, we initialize the component and try to read it if in read-mode.
From the second time forward a call of the property returns the initialized component without trying to read.
Now we can use the set_ methods again inside the read_ methods like we used to do.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Tests & pre-commit hooks pass
  • Updated documentation if needed
  • Updated changelog.rst if needed

Additional Notes (optional)

This could also break code downstream, if the internal properties are called (e.g. self._maps instead of self.maps). In the new implementation calling the internal static properties should be restricted to the set_ methods and the dynamic property methods only which happen in the core. Hence, an easy fix should be to replace all calls to the internal static property by the dynamic property.

@DirkEilander DirkEilander changed the title initialize model components with None Avoid RecursionError when using Model.set methods inside Model.read methods Aug 9, 2023
Copy link
Contributor

@savente93 savente93 left a comment

Choose a reason for hiding this comment

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

Good to see that you added a test for this. I see nothing out of the ordinary, so if this test reproduces the problem and passes with this patch then it should be ready to merge!

Copy link
Contributor

@hboisgon hboisgon left a comment

Choose a reason for hiding this comment

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

Nice and more elegant way to fix model reading methods :) with this we might be changing behavior on what to expect with an empty object (eg you had to modify self.res and self.dims etc) so these changes may affect the plugins. So I think good to document the changes in the changelog.
I also discussed with @DirkEilander that the changes for Mesh will be applied in PR #412 after this has been merged.

So good to merge once the changelog is updated :)

@DirkEilander DirkEilander merged commit df18641 into main Aug 9, 2023
7 checks passed
@DirkEilander DirkEilander deleted the fix/set_recursion branch August 9, 2023 11:09
savente93 pushed a commit that referenced this pull request Aug 10, 2023
…ethods (#473)

* initialize model components with None

* improve tests; small fix in GridModel.write

* update changelog
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.

The recent update in #460 breaks HydroMT-SFINCS
3 participants