Repository navigation
Conversation
|
| :meth:`imod.formats.isg.ISG.read`, including segment geometry, cross-sections | ||
| and plots. | ||
| - :func:`imod.data.isg_tutorials` to load the SFR and LAK tutorials of the iMOD | ||
| User Manual, with ISG-files. |
There was a problem hiding this comment.
iMOD Python currently doesn't support the SFR package for MODFLOW 6. So mentioning that will lead to confusion, I reckon. I'd keep it more generic:
:func:imod.data.isg_tutorials to load example ISG data from the iMOD5 tutorials.
JoerivanEngelen
left a comment
JoerivanEngelen
left a comment
There was a problem hiding this comment.
My review pt 2. Will continue later
| isg.ISG | ||
| isg.ISP | ||
| isg.ISD1 | ||
| isg.RivISD2 | ||
| isg.SfrISD2 | ||
| isg.ISC1 | ||
| isg.ISC2 | ||
| isg.IST1 | ||
| isg.IST2 | ||
| isg.ISQ1 | ||
| isg.ISQ2 |
There was a problem hiding this comment.
If you compare this to the other functions in the formats API, this is different. I don't really like the allcaps here, that makes it look like a constant. Up to now we didn't expose constants to the public API (That is: They are not in the documentation)
There was a problem hiding this comment.
It's quite a departure of how we implemented the readers for the IPF and IDF files, but I think this looks quite neat. Our users will appreciate the plotting and geometry functions.
| """ | ||
| Number of data records in the ISD2-file that describes the timeserie of the | ||
| calculation point. | ||
| """ |
There was a problem hiding this comment.
I've never seen this kind of documentation to document dataclass variables. Is this common practice? We normally go for listing them under a PARAMETERS header in the main docstring.
There was a problem hiding this comment.
I think this is quite a clean implementation, with some cool features for users: the quick access to geometries and plotting.
(I was thinking, this might be an interesting data model for the IPF file (iMOD Point File) as well. But might be overkill)
My main gripes are the following:
- The docstring style of the dataclass differs from the way dataclass docstrings are implemented elsewhere, especially the arguments. Could you change that?
- This is quite a different API than our users are used to. I think they would expect something like a function like
imod.isg.read(path)to read their files. This would then return our custom dataclass instead of the vanilla geodataframe/dataframe/DataArray/Dataset that they are used to. Adding examples in the docstrings for the documentation, as well as adding examples toexamples/user-guide/11-imod5-files.py - Some of the implementation notes added by the LLM to the docstring add confusion, as they are conflicting with previous text from the iMOD5 docs. In that case, I'd just like to know the truth, and a small internal note that the iMOD5 docs said something different which turned out to be untrue.
| "pandas", | ||
| "pooch", | ||
| "pydantic", | ||
| "pydantic>=2.7", |
There was a problem hiding this comment.
The developer env is specified in the pixi.toml, you might consider also adding setting this there to properly generate the pixi.lock. I see an update of the pixi.lock file, but no pixi.toml?
| iMOD_User_Manual*.pdf | ||
| *.[Ii][Ss][GgPp] | ||
| *.[Ii][Ss][DdCcTtQq][12] |
There was a problem hiding this comment.
Are these files just for personal testing or generated in tests? I think we can leave this out of .gitignore, now that you are done, right?
| for entry in system_entries: | ||
| if entry["path"] not in models: | ||
| models[entry["path"]] = _try_read_with_func(ISG.read, entry["path"]) | ||
| time, repeats = _process_ipf_time(entry, periods, repeats) |
There was a problem hiding this comment.
Great that you could reuse _process_ipf_time here, perhaps good to rename this function than to something like _process_ipf_isg_time (I couldn't come up with a better name now, but hopefully you can think of something better)
| Zm + Zc / 100. Implementation note: Zc carries the sign, e.g. -23.43 is | ||
| stored as Zm=-23, Zc=-43. |
There was a problem hiding this comment.
Looking at the example: he sign is in both Zm and Zc? That doesn't seem to be implied by "Zc carries the sign".
| ctime: Vector[np.str_] | None = None | ||
| """ | ||
| Time representation as hh:mm:ss. This additional record is only applicable | ||
| for double precision ISD2-files. It is not used yet! |
There was a problem hiding this comment.
What do you mean with "It is not used yet!" ? And when is it used?
| Maximum size of each label is 52 characters. | ||
|
|
||
| Implementation note: iMOD reads 50 characters. |
There was a problem hiding this comment.
So which one is it in the end?
| return self.geodataframe().plot(ax=ax, **kwargs) | ||
|
|
||
| @property | ||
| def __geo_interface__(self) -> dict[str, Any]: |
There was a problem hiding this comment.
I think this comes from https://geopandas.org/en/stable/docs/reference/api/geopandas.GeoDataFrame.__geo_interface__.html, but for what is this necessary? Do we need the magic method here to properly work with geopandas for plotting or something? If so, add that to the docstring please.
| return start + offset | ||
|
|
||
|
|
||
| def to_datetime(idate: np.ndarray, ctime: np.ndarray | None = None) -> pd.DatetimeIndex: |
There was a problem hiding this comment.
Nitpick: There already is to_datetime function under imod.util.time (which is public API), which is called in quite a few places (via to_datetime_internal). Consider renaming this function (though it is under a different namespace) to distinguish it and avoid confusion.
Fixes #326
Description
This adds an ISG reader in the hydrolib/geolib/ribasim style. That is, each file is represented by one class, as closely as possible. Responsibilities are abstracted into mixin classes and sometimes inherited. Details are in the
README.mdfile. I've copied most docstrings (per method/attribute) directly from the pdf.The ISG is read when present in a project, but it isn't (cannot be) used in writing.
I've used the iMOD tutorials for testing, I still need to