Deltares / imod-python

🐍🧰 Make massive MODFLOW models
https://deltares.github.io/imod-python/
MIT License
16 stars 0 forks source link

Make a principled decision on None versus "none" in MF6 options #1024

Open Huite opened 2 months ago

Huite commented 2 months ago

I just noticed that in the imod.mf6.Solution class there are some ambiguities when it comes to None options.

Options like reordering_method or print_option have as acceptable MF6 values: {NONE, RCM, MD} and {NONE, SUMMARY, ALL} respectively (and we prefer lower case).

In many cases, NONE is the default option. Which means that you may omit it from the MF6 IMS file. However, the print_option has as its default value "summary". So to silence it, you need to set to ensure PRINT OPTION NONE is written into the file, which requires providing the value "none" as a string.

To demonstrate:

        imod.mf6.Solution(
            modelnames=["GWF_1"],
            outer_dvclose=4,
            outer_maximum=500,
            inner_dvclose=1.0e-4,
            inner_rclose=0.001,
            inner_maximum=100,
            linear_acceleration="cg",
            print_option=None,
        )

To actually silence, you need:

```python
        imod.mf6.Solution(
            modelnames=["GWF_1"],
            outer_dvclose=4,
            outer_maximum=500,
            inner_dvclose=1.0e-4,
            inner_rclose=0.001,
            inner_maximum=100,
            linear_acceleration="cg",
            print_option="none",
        )

Results in no entry being written in the IMS file (due to how the jinja rendering works), which means MF6 will default to SUMMARY instead.

In other cases, allowing non-string None works fine because it results in no value being written --> mf6 uses the NONE default. Really, the issue is that MF6 choose the word None rather than Null or whatever, and None happens to be a Python keyword. Matplotlib has something similar, where if you don't want to show faces or edges on a plot, you set facecolor="none".

I've added an OptionSchema in #1018. It currently ignores None values, since technically many of our options currently have None as a default argument. Perhaps the easiest solution is the following:

This will still result in some frustration as people will likely write None instead of "none". The OptionSchema validation error should probably clarify the type though since it doesn't communicate the difference nicely:

    - Invalid option: None. Valid options are: none, summary, all

Since it will lower strings otherwise, so "None" would be allowed.

Maybe:

            expected = type(next(iter(self.options)))
            raise ValidationError(
                f"Invalid option: {value} of type {type(value).__name__}. Valid options are: {valid_values} of type {expected.__name__}"
            )

This only works if not multiple types are allowed.

(The DTypeSchema won't suffice here btw since it allows None values.)

Defaulting to "none" as a string has the added benefit of a more verbose and more descriptive IMS file -- this is somewhat philosophical and maybe warrants a paragraph in the developers docs: obviously MF6 treats its input files as a user facing, and they don't want to burden the user with filling in these values. However, to us the Python API is user facing and the files are just data transfer.