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

The recent update in #460 breaks HydroMT-SFINCS #472

Closed
2 tasks done
DirkEilander opened this issue Aug 8, 2023 · 0 comments · Fixed by #473
Closed
2 tasks done

The recent update in #460 breaks HydroMT-SFINCS #472

DirkEilander opened this issue Aug 8, 2023 · 0 comments · Fixed by #473
Labels
Bug Something isn't working

Comments

@DirkEilander
Copy link
Contributor

HydroMT version checks

  • I have checked that this issue has not already been reported.
  • I have checked that this bug exists on the latest version of HydroMT.

Reproducible Example

from hydromt import Model, raster

class MyModel(Model):
    def __init__(self, root=None, mode="r"):
        super().__init__(root, mode=mode)

    def read_maps(
        self,
    ) -> None:
        da = raster.full_from_transform(
            transform=[0.5, 0.0, 3.0, 0.0, -0.5, -9.0],
            shape=(4, 6),
        )
        # this will cause a recursion error in read-mode
        self.set_maps(da, name="test")
        # and is preferred over the current implementation
        # which does not make use of the checks in set_maps method
        self._maps["test"] = da

mod = MyModel(mode="r+")
mod.read_maps() # RecursionError 

Current behaviour

You get a RecursionError when using set methods inside read methods for any model component since #460
This is used a lot in plugins and the new implementation causes errors downstream, see e.g. https://github.com/Deltares/hydromt_sfincs/actions/runs/5797018073/job/15711743003

Desired behaviour

I prefer to use the set_ methods inside the read methods to avoid duplicating code that does checks on the data. For developers implementing hydromt for their model I've always explicitly advised to avoid setting directly using self._maps[name] = data for the same reason.

Additional context

No response

@DirkEilander DirkEilander added Bug Something isn't working Needs refinement issue still needs refinement labels Aug 8, 2023
@DirkEilander DirkEilander removed the Needs refinement issue still needs refinement label Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant