-
Notifications
You must be signed in to change notification settings - Fork 3
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
Milestone
Comments
JoerivanEngelen
changed the title
Refactor Package.write
Remove Sep 17, 2024
WriteContext
from public API
5 tasks
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
5 tasks
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
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
. TheWriteContext
is passed through from Simulation to Model to Package, and accepted as argument for thePackage.write
class. However, this altered the public API, as now users have to first create aWriteContext
, 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:
The text was updated successfully, but these errors were encountered: