Issue #1738 add ignore time arguments

[JoerivanEngelen]

Fixes #1738

Description

• Adds missing ignore_time_purge_empty arguments to allow users to ignore time
• Remove underscore of two functions which were used outside the module, as this apparantly is not pythonic

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
• If feature added: Added feature to API documentation
• If pixi.lock was changed: Ran pixi run generate-sbom and committed changes

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Manangka]

I find this a very confusing name.

I think that we need to keep the user away from the internals of this method. They shouldn't have to know that packages are being purged.

Would ignore_time_validation work? Or have them provide a ValidationContext?

[JoerivanEngelen]

Legitimate critique. Unfortunately there are already some methods with this argument as name in the public API, so if I change it here, I have to change it everywhere, and keep on supporting the old argument for the next versions to keep the API stable.

Not the end of the world, but perhaps out of scope for this PR?

I want to refrain from letting users provide ValidationContext themselves, as limiting the amount of custom objects users have to create themselves decreases the learning curve for iMOD Python.

[Manangka]

I first thought that this method was calling itself but it is actually calling a utilities method.
Would it be clearer to add the module name to it?:
utitlities.mas. mask_all_packages

Same applies to mask_all_models

[JoerivanEngelen]

Right, that might have been part of the reason the underscore was left there in the first place.

I tried the following:

1. Import the utilities module

from  imod.common import utilities as common_utilities
...
    common_utilities.mask.mask_all_packages(...)

Pythons runs this without error, but this was not understood by the linters and VSCode couldn't trace the function for some reason.

2. Import the mask utilities module

import imod.common.utilities as utilities
...
def mask_all_packages(mask, ...)
    ...
    mask.mask_all_packages(mask, ...)

The function also has mask as argument here, so the module name clashes here with the argument name.

3. Import under alias

import imod.common.utilities.mask as mask_utils
...
    mask_utils.mask_all_packages(...)

This too can lead to confusion, as it can lead to developers looking for a module file/folder named mask_utils which is not present (if they overlook the aliasing in the imports). Given that this way of importing is done nowhere else in the codebase, I deem this confusion quite likely.

4. Reference all modules in function call

import imod
...
    imod.common.utilities.mask.mask_all_packages(...)

Too much clutter in my opinion.

I think I keep it as is in this PR, downside is that developers have to be sharp for the difference between function and method calls here. It appears changing it back to how it was (as a function starting with an underscore) and using it outside the module is a no no in the python world.

[Manangka]

I think this explanation can use some improvement.
Maybe elaborate more on the fact that after clipping packages can become empty e.g. ...
An package is considered empty when ...
This boolean determines if the time dimension is taken into account when determining empty packages

[Manangka]

In all the test it doesnt matter what the flag does
It would be good to have some tests where the outcome is different depending on the flag