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

API inconsistency regarding layer coords and MetaSWAP .from_imod5_data methods #1336

Open
JoerivanEngelen opened this issue Dec 13, 2024 · 0 comments

Comments

@JoerivanEngelen
Copy link
Contributor

#1335 will implement an imod.msw.MetaSwapModel.from_imod5_data method. What is not tackled here is the following:

  1. imod.prj.open_projectfile_data returns grids for the CAP package with a layer coordinate
  2. This coordinate information can be potentially required for when coupling MetaSwapModels to other layers than layer 1 (when this is implemented)
  3. Right now, imod.msw.MetaSwapModel.from_imod5_data drops this information
  4. The package.from_imod5_data implementations require the layer coord to be dropped

This makes for inconsistent API: imod.msw.MetaSwapModel.from_imod5_data is OK with the layer coord that is always added by open_projectfile_data, whereas e.g. imod.msw.GridData.from_imod5_data requires it to be dropped.

This makes for an annoying inconsistency in the API, for when users try to manually import individual packages themselves. I don't see a big use-case for it, but it IS unexpected behaviour.

To get something which works is very easy: xarray has an errors: "ignore" argument for its xr.DataArray.isel method. My concern is more a matter of performance: I haven't tested yet what xarray does when it ignores errors: Does it return a copy or a view? If the first, this would mean that potentially a lot of data would be copied, as the same grids are used for different packages, therefore grids would be copied multiple times for each package. Some profiling on the LHM data would be good to do here to test whether this leads to big bottlenecks.

@github-project-automation github-project-automation bot moved this to 📯 New in iMOD Suite Dec 13, 2024
@JoerivanEngelen JoerivanEngelen changed the title imod.msw.MetaSwapPackage.from_imod5_data requires users to drop the layer coord, whereas imod.msw.MetaSwapModel.from_imod5_data doesn't MetaSwapPackage.from_imod5_data requires users to drop the layer coord, whereas MetaSwapModel.from_imod5_data doesn't Dec 13, 2024
@JoerivanEngelen JoerivanEngelen changed the title MetaSwapPackage.from_imod5_data requires users to drop the layer coord, whereas MetaSwapModel.from_imod5_data doesn't API inconsistency regarding layer coords and MetaSWAP .from_imod5_data methods Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📯 New
Development

No branches or pull requests

1 participant