Deltares / Deltares/imod-python

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

Open
#1,024 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
41
Forks
12
Avg merge
21h 8m
Merged PRs (30d)
1

Description

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:

  • Do not allow None in the OptionSchema validate.
  • Update the default argument for these options where they are currently None, and replace them by "none".
  • Add "none" to the allowed values in the schema.
  • Update docstrings carefully, explicitly requiring string entries.

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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with imod.mf6.Solution and the OptionSchema added in #1018, then compare how print_option and reordering_method are rendered into the IMS file. Review the DTypeSchema, related defaults, validation error wording, docstrings, and developer documentation; the work is done when the None versus "none" behavior and its documentation are decided and applied consistently.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.