-
Notifications
You must be signed in to change notification settings - Fork 7
Feature: optional loading of an EPOCH input deck #93
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @Dartrisen, thank you very much for implementing the new feature! This is a great first start although it would be nice to also allow for this parameter would accept On top of adding this feature could you also add some additional documentation to highlight how this works? Probably a small section after this https://sdf-xarray.readthedocs.io/en/latest/key_functionality.html#loading-specific-variables or potentially a new page if you prefer. |
src/sdf_xarray/__init__.py
Outdated
| if self.load_deck: | ||
| import epydeck # type: ignore # noqa: PGH003 | ||
| deck_path = Path(attrs["filename"]).parent / "input.deck" | ||
| with open(deck_path, encoding="utf-8") as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| with open(deck_path, encoding="utf-8") as f: | |
| with open(deck_path) as f: |
utf-8 encoding specification is not required here
|
FYI, you might want to additionally pull in branch #89 so that it works with datatrees as well |
JoelLucaAdams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feature implementation! There are a few things that need ironing out that I have listed below.
On top of those changes you also need to add some tests please
| togglebutton_hint = "Click to show input.deck" | ||
| togglebutton_hint_hide = "Click to hide input.deck" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this changes the toggle information globally I think it's best if it's removed
| When loading SDF files, you can optionally include the `input.deck` file | ||
| used to generate the simulation. This file contains important simulation | ||
| parameters that may not be present in the SDF outputs. By loading this file, | ||
| you can access these parameters as part of your dataset's metadata. | ||
| To do this, use the ``input_deck`` parameter when calling | ||
| `sdf_xarray.open_dataset`. | ||
|
|
||
| .. note:: | ||
| You may need to install the `epydeck` extra dependencies | ||
| to enable this functionality. You can do this via pip: | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| pip install "epydeck" | ||
|
|
||
| By default the ``input_deck`` parameter is set to ``False``. You can set it to | ||
| ``True`` to automatically search for an `input.deck` file in the same directory | ||
| as the SDF file being loaded, or you can provide a specific path to the | ||
| `input.deck` file as a path variable or string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Watch out when using single "`" backticks here as they always need to be double unless you're referencing an internal function or one of the ones linked in the conf.py.
| .. note:: | ||
| You may need to install the `epydeck` extra dependencies | ||
| to enable this functionality. You can do this via pip: | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| pip install "epydeck" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .. note:: | |
| You may need to install the `epydeck` extra dependencies | |
| to enable this functionality. You can do this via pip: | |
| .. code-block:: bash | |
| pip install "epydeck" | |
| In order to parse the deck file you need to install the | |
| `epydeck` package. This can be done via pip: | |
| .. code-block:: bash | |
| pip install epydeck |
I don't think this needs to be a note as its required in order to work but feel free to leave it in if you think otherwise. Even so I think the wording needs to change slightly to look more like this
| By default the ``input_deck`` parameter is set to ``False``. You can set it to | ||
| ``True`` to automatically search for an `input.deck` file in the same directory | ||
| as the SDF file being loaded, or you can provide a specific path to the | ||
| `input.deck` file as a path variable or string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| By default the ``input_deck`` parameter is set to ``False``. You can set it to | |
| ``True`` to automatically search for an `input.deck` file in the same directory | |
| as the SDF file being loaded, or you can provide a specific path to the | |
| `input.deck` file as a path variable or string. | |
| By default the ``input_deck`` parameter is set to ``False``. You can set it to | |
| ``True`` to automatically search for a file with the name ``"input.deck"`` in the | |
| same directory as the SDF file being loaded. If you wish to load a file that has | |
| a different name and the path is relative then it will search in the same directory | |
| as the SDF file (e.g. ``"template.deck"``), if it is absolute then it will use the | |
| full path to the find the file (e.g. ``"/path/to/decks/input.deck"``). |
Slight change to the description here
src/sdf_xarray/__init__.py
Outdated
| keep_particles=False, | ||
| lock=None, | ||
| probe_names=None, | ||
| load_deck: bool | str | Path = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| load_deck: bool | str | Path = False, | |
| load_deck: bool | PathLike = False, |
Use the PathLike defined on line 43
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt that it is more readable than str | Path, especially if it wasn't defined like a default type in Path lib but fixed like you suggested.
src/sdf_xarray/__init__.py
Outdated
|
|
||
| sdf_dir = Path(attrs["filename"]).parent | ||
|
|
||
| if isinstance(self.load_deck, (str, Path)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if isinstance(self.load_deck, (str, Path)): | |
| if isinstance(self.load_deck, PathLike): |
Again, use the PathLike type from above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, fixed
| def open_datatree( | ||
| self, | ||
| filename_or_obj, | ||
| *, | ||
| drop_variables=None, | ||
| keep_particles=False, | ||
| probe_names=None, | ||
| ): | ||
| ds = self.open_dataset( | ||
| filename_or_obj, | ||
| drop_variables=drop_variables, | ||
| keep_particles=keep_particles, | ||
| probe_names=probe_names, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you're missing a load_deck in here so that it's passed into the dataset
| path: PathLike, | ||
| *, | ||
| keep_particles: bool = False, | ||
| probe_names: list[str] | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be updated so that it includes load_deck and simply passes it into xr.open_datatree(...) as its handled in SDFEntryPoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should also be an implementation of load_deck in open_mfdataset(...) and open_mfdatatree(...). Since both of these along with the base implementation in SDFDataStore.load(...) will use the same underlying structure I'd probably extract the logic to a separate hidden helper function at the top of the file e.g. _load_deck(...).
| parameters that may not be present in the SDF outputs. By loading this file, | ||
| you can access these parameters as part of your dataset's metadata. | ||
| To do this, use the ``input_deck`` parameter when calling | ||
| `sdf_xarray.open_dataset`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you've implemented load_deck into the other classes then please update this to link to them e.g.
| `sdf_xarray.open_dataset`. | |
| `xarray.open_dataset`, `sdf_xarray.open_datatree`, `sdf_xarray.open_mfdataset` and `sdf_xarray.open_mfdatatree`. |
Now you can get access to the input.deck file by simply doing this:
The package
epydeckshould be installed in your virtual environment to use this. The default value ofload_deckisFalse.Resolves #91