Issue #1041 import simulation #1059

luitjansl · 2024-05-27T14:01:07Z

Description

Adds a function that imports a simulation from an imod5 project file. It creates a simulation, containing 1 GroundwaterFlowModel, containing the packages for which we support importing currently. This includes the mandatory flow packages like NPF and STO, and some boundary conditions (like DRN and RCH) but not yet CHD or WEL.

Checklist

Links to correct issue
Update changelog, if changes affect users
PR title starts with Issue #nr, e.g. Issue #737
Unit tests were added
If feature added: Added/extended example

…mport_recharge

review comment Co-authored-by: Joeri van Engelen <[email protected]>

…rt_simulation

…version # Conflicts: # imod/mf6/rch.py # imod/tests/fixtures/backward_compatibility_fixture.py # imod/tests/test_mf6/test_mf6_rch.py # imod/tests/test_typing/test_typing_grid.py # imod/typing/grid.py

review comment Co-authored-by: Joeri van Engelen <[email protected]>

…version

…_simulation # Conflicts: # imod/tests/test_typing/test_typing_grid.py # imod/typing/grid.py

review comment Co-authored-by: Joeri van Engelen <[email protected]>

…es/imod-python into issue_#967_drn_conversion

…lation

…_simulation

JoerivanEngelen

Looks good, approving in advance, since I'm not going to be so active the coming 10 days. My main points are the quite long variable names for the simulation options and the missing docstrings, could you fix/implement those?

JoerivanEngelen · 2024-05-28T09:17:43Z

imod/mf6/model_gwf.py

+        default_simulation_allocation_options: SimulationAllocationOptions,
+        default_simulation_distributing_options: SimulationDistributingOptions,
+        regridder_types: Optional[dict[str, tuple[RegridderType, str]]] = None,
+    ) -> "GroundwaterFlowModel":


Please add a docstring to the method, explaining what it does

done, added the docstring

JoerivanEngelen · 2024-05-28T09:28:12Z

imod/mf6/model_gwf.py

+        default_simulation_allocation_options: SimulationAllocationOptions,
+        default_simulation_distributing_options: SimulationDistributingOptions,


These variable names are quite long. I think going with this is fine:

Suggested change

default_simulation_allocation_options: SimulationAllocationOptions,

default_simulation_distributing_options: SimulationDistributingOptions,

allocation_options: SimulationAllocationOptions,

distributing_options: SimulationDistributingOptions,

The type annotation hints that we are expecting a certain object and you can describe this also in the docstring. Furthermore, the plural options subtly hints that this is something different then option used for specific packages.

JoerivanEngelen · 2024-05-28T09:32:22Z

imod/mf6/model_gwf.py

+        # import npf
+        npf_pkg = NodePropertyFlow.from_imod5_data(imod5_data, grid, regridder_types)
+
+        # import sto
+        sto_pkg = StorageCoefficient.from_imod5_data(imod5_data, grid, regridder_types)
+
+        # import initial conditions
+        ic_pkg = InitialConditions.from_imod5_data(imod5_data, grid, regridder_types)
+
+        # import recharge
+        rch_pkg = Recharge.from_imod5_data(imod5_data, dis_pkg, regridder_types)


Nice, this looks very clean and readable!

JoerivanEngelen · 2024-05-28T09:45:49Z

imod/mf6/model_gwf.py

+        oc = OutputControl(save_head="last", save_budget="last")
+        result["oc"] = oc


To be more consistent with iMOD5, we can make the user responsible for this.

JoerivanEngelen · 2024-05-28T09:48:16Z

imod/mf6/simulation.py

+        default_simulation_allocation_options: SimulationAllocationOptions,
+        default_simulation_distributing_options: SimulationDistributingOptions,


idem, long argument name.

JoerivanEngelen · 2024-05-28T09:48:35Z

imod/mf6/simulation.py

+        default_simulation_allocation_options: SimulationAllocationOptions,
+        default_simulation_distributing_options: SimulationDistributingOptions,
+        regridder_types: Optional[dict[str, tuple[RegridderType, str]]] = None,
+    ) -> "Modflow6Simulation":


Please also add a docstring for this method.

JoerivanEngelen · 2024-05-28T09:49:58Z

imod/prepare/topsystem/default_allocation_methods.py

+@dataclass()
+class SimulationAllocationOptions:


Looks good!

JoerivanEngelen · 2024-05-28T09:53:15Z

imod/prepare/topsystem/default_allocation_methods.py

@@ -0,0 +1,40 @@
+from dataclasses import dataclass


FYI: changing the dataclass to pydantic dataclass, would be as easy as:

Suggested change

from dataclasses import dataclass

from pydantic.dataclasses import dataclass

You can make this change when PR #1054 is merged, probably not for this PR.

JoerivanEngelen · 2024-05-28T09:56:09Z

imod/tests/test_mf6/test_mf6_simulation.py

+
+
+@pytest.mark.usefixtures("imod5_dataset")
+def test_import_from_imod5(imod5_dataset, tmp_path):


Are we also going to add a test to see if it runs? Or do we assume our validation is good enough to capture all input mistakes, and save some precious CI time? I'm open to both options.

I tried if it runs (it does), but I didn't add it to the test because that adds 20 seconds or so to the test run time.

luitjansl and others added 30 commits May 8, 2024 17:38

work started

5595422

recharge import with tests

e40f606

recharge import with tests

a88bcfb

now supports transient data

fdf5379

added test for transient

a48bb19

fixed tests

782abc5

formatting

5bc04e6

added unittests for grid functions

868aa30

reordering

bf3fb63

format

6096c8e

comments

8781c25

comments

d1d1200

work started

39cdad0

recharge import with tests

c0f7059

recharge import with tests

10d6b65

now supports transient data

27263ff

added test for transient

7be0cd5

fixed tests

d4aea05

formatting

8338df3

added unittests for grid functions

e29212b

reordering

ed75148

format

c17da61

comments

50c871b

comments

e816f79

work started

4f3e6b5

Merge branch 'imod5_converter_feature_branch' into issue_#969_imod5_i…

3e48a03

…mport_recharge

work in progress

b905483

Update imod/typing/grid.py

dafba74

review comment Co-authored-by: Joeri van Engelen <[email protected]>

work in progress

7c5d077

cleanup

4094bd2

luitjansl and others added 20 commits May 21, 2024 14:28

Merge branch 'issue_#969_imod5_import_recharge' into issue_#1041_impo…

58c84aa

…rt_simulation

adding test

0fb9fc4

Merge branch 'imod5_converter_feature_branch' into issue_#967_drn_con…

23423ff

…version # Conflicts: # imod/mf6/rch.py # imod/tests/fixtures/backward_compatibility_fixture.py # imod/tests/test_mf6/test_mf6_rch.py # imod/tests/test_typing/test_typing_grid.py # imod/typing/grid.py

Update imod/mf6/drn.py

d330f6c

review comment Co-authored-by: Joeri van Engelen <[email protected]>

Update imod/mf6/drn.py

a570a48

review comment Co-authored-by: Joeri van Engelen <[email protected]>

now imports simulation

ea3e0e4

review comments

a46f28c

cleanup

5f53d76

Merge branch 'imod5_converter_feature_branch' into issue_#967_drn_con…

538906c

…version

Merge branch 'imod5_converter_feature_branch' into issue_#1041_import…

218bb3e

…_simulation # Conflicts: # imod/tests/test_typing/test_typing_grid.py # imod/typing/grid.py

Update imod/mf6/drn.py

50c89d1

review comment Co-authored-by: Joeri van Engelen <[email protected]>

Update imod/mf6/drn.py

eda42ca

review comment Co-authored-by: Joeri van Engelen <[email protected]>

Update imod/tests/test_mf6/test_mf6_drn.py

d0f498a

review comment Co-authored-by: Joeri van Engelen <[email protected]>

review comment

493b524

Merge branch 'issue_#967_drn_conversion' of https://github.com/Deltar…

b43d545

…es/imod-python into issue_#967_drn_conversion

Merge branch 'issue_#967_drn_conversion' into issue_#1041_import_simu…

44cfa9b

…lation

Merge branch 'imod5_converter_feature_branch' into issue_#1041_import…

79c4fdd

…_simulation

added drn to simulation import

1866150

formatting

5385810

formatting and adding test

ad7baf7

luitjansl changed the base branch from master to imod5_converter_feature_branch May 27, 2024 14:01

luitjansl added 2 commits May 27, 2024 16:06

docstrings

ce2acdc

cleanup

7f0fe7f

luitjansl requested a review from JoerivanEngelen May 27, 2024 14:12

JoerivanEngelen approved these changes May 28, 2024

View reviewed changes

luitjansl added 3 commits May 28, 2024 15:04

docstring

819167b

review comments

8010edc

review comments

d751d88

luitjansl merged commit a31d86f into imod5_converter_feature_branch May 30, 2024
7 checks passed

luitjansl deleted the issue_#1041_import_simulation branch May 30, 2024 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #1041 import simulation #1059

Issue #1041 import simulation #1059

luitjansl commented May 27, 2024

JoerivanEngelen left a comment

JoerivanEngelen May 28, 2024

luitjansl May 28, 2024

JoerivanEngelen May 28, 2024

luitjansl May 28, 2024

JoerivanEngelen May 28, 2024

JoerivanEngelen May 28, 2024

luitjansl May 28, 2024

JoerivanEngelen May 28, 2024

luitjansl May 28, 2024

JoerivanEngelen May 28, 2024

luitjansl May 28, 2024

JoerivanEngelen May 28, 2024

JoerivanEngelen May 28, 2024

JoerivanEngelen May 28, 2024

luitjansl May 28, 2024

		default_simulation_allocation_options: SimulationAllocationOptions,
		default_simulation_distributing_options: SimulationDistributingOptions,

		oc = OutputControl(save_head="last", save_budget="last")
		result["oc"] = oc

	from dataclasses import dataclass
	from pydantic.dataclasses import dataclass



		@pytest.mark.usefixtures("imod5_dataset")
		def test_import_from_imod5(imod5_dataset, tmp_path):

		@dataclass()
		class SimulationAllocationOptions:

Issue #1041 import simulation #1059

Issue #1041 import simulation #1059

Conversation

luitjansl commented May 27, 2024

Description

Checklist

JoerivanEngelen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment