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

Remove WriteContext from public API #1223

Closed
JoerivanEngelen opened this issue Sep 17, 2024 · 0 comments · Fixed by #1278
Closed

Remove WriteContext from public API #1223

JoerivanEngelen opened this issue Sep 17, 2024 · 0 comments · Fixed by #1278
Assignees
Milestone

Comments

@JoerivanEngelen
Copy link
Contributor

JoerivanEngelen commented Sep 17, 2024

Somewhere around 5523c24 the WriteContext was introduced, this is an object to collect settings for writing (directory, save as binary, etc.). This is generated internally when calling Modflow6Simulation.write. The WriteContext is passed through from Simulation to Model to Package, and accepted as argument for the Package.write class. However, this altered the public API, as now users have to first create a WriteContext, an object stowed away in a non-public module, to be able to write their packages separately.

I'm not sure how large the use-case is, but it the write context is quite an extra hurdle for users now to call a method that is part of the public API. I like the idea of the context, so to work around this, we could make the current write a semi-private _write, and create a separate method for the public API which collects all settings and calls _write. Modflow6Simulation.write would then directly call _write.

The only downside of this is that extending the WriteContext with some extra settings, might require some extra work, as arguments have to be added as well. However, I think that is worth it now, as letting users create internal objects to do what they want to do is also unwanted.

Something like:

class Package:
...

    def write(
        self,
        pkgname: str,
        globaltimes: Union[list[np.datetime64], np.ndarray],
        directory: str,
        binary: bool = True,
        use_absolute_paths = False,
    ):
       write_context = WriteContext(directory, binary, use_absolute_paths)
       self._write(pkgname, globaltimes, write_context)

    @standard_log_decorator()
    def _write(
        self,
        pkgname: str,
        globaltimes: Union[list[np.datetime64], np.ndarray],
        write_context: WriteContext,
    ):
        directory = write_context.write_directory
        binary = write_context.use_binary
        self.write_blockfile(pkgname, globaltimes, write_context)

        if hasattr(self, "_grid_data"):
            if self._is_xy_data(self.dataset):
                pkgdirectory = directory / pkgname
                pkgdirectory.mkdir(exist_ok=True, parents=True)
                for varname, dtype in self._grid_data.items():
                    key = self._keyword_map.get(varname, varname)
                    da = self.dataset[varname]
                    if self._is_xy_data(da):
                        if binary:
                            path = pkgdirectory / f"{key}.bin"
                            self.write_binary_griddata(path, da, dtype)
                        else:
                            path = pkgdirectory / f"{key}.dat"
                            self.write_text_griddata(path, da, dtype)
@JoerivanEngelen JoerivanEngelen changed the title Refactor Package.write Remove WriteContext from public API Sep 17, 2024
JoerivanEngelen added a commit that referenced this issue Sep 18, 2024
Fixes #1222

# Description
- Add a ``ValidationContext`` dataclass
- Add a ``_validation_context`` attribute to ``Modflow6Simulation``;
this replaces the ``_is_from_imod5`` attribute.
- The ``ValidationContext`` contains an attribute for strict well
checks, turned on by default. This is set to False when calling
``from_imod5`` or for split simulations.
- Adds a ``_to_mf6_pkg`` method in a similar design as proposed in
#1223, this to preserve public API.
- Refactor ``WriteContext``, to make it a dataclass again. I had to
ignore type annotation for ``write_directory``, otherwise MyPy would
throw errors. The whole property shebang presumably started with MyPy
throwing errors. Reverting it back to a dataclass reduces the lines of
code considerably, which makes it more maintainable.
- Use jit for examples run, this speeds them up considerably. The
examples ran into a TimeOut on TeamCity, and this reduces the change of
that happening again.

# Checklist
<!---
Before requesting review, please go through this checklist:
-->

- [x] Links to correct issue
- [x] Update changelog, if changes affect users
- [x] PR title starts with ``Issue #nr``, e.g. ``Issue #737``
- [ ] Unit tests were added
- [ ] **If feature added**: Added/extended example
@JoerivanEngelen JoerivanEngelen added this to the v1.0 release milestone Oct 21, 2024
@JoerivanEngelen JoerivanEngelen self-assigned this Nov 8, 2024
github-merge-queue bot pushed a commit that referenced this issue Nov 8, 2024
Fixes #1223

# Description
Remove WriteContext and ValidationContext from public API. 

# Checklist
<!---
Before requesting review, please go through this checklist:
-->

- [x] Links to correct issue
- [x] Update changelog, if changes affect users
- [x] PR title starts with ``Issue #nr``, e.g. ``Issue #737``
- [ ] Unit tests were added
- [ ] **If feature added**: Added/extended example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

1 participant