Deltares / imod-python

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

Consider refactoring regridder options, to reduce code redundancy #416

Closed Manangka closed 10 months ago

Manangka commented 10 months ago

In GitLab by @JoerivanEngelen on Jun 1, 2023, 16:38

There now is a function within regridding_utils, named create_regridder_from_string. THis is used when users provide their own regridder options per variable to package. My suggestion was to let users immediately provide the regridder objects themselves e.g. xu.BaryCentricInterpolator, instead of a string ``"BaryCentricInterpolator" and then have a function which matches all these strings.

import xugrid as xu

_regrid_options = {"var1": (xu.OverlapRegridder, "mean")}
regridder_instances = {}

for name, (cls, method) in _regrid_options:
    regridder_instances[name] = cls(src_grid, target_grid, method)

The benefit of this is that it makes create_regridder_from_string redundant, and users can use their IDE's to see if they didn't make a typo in providing the right regridder object. This was not done, because in the first merge of this user story, it was assumed that names of objects might change, and that this will lead to backwards compatibility issues for users. This assumption is irrelevant, as

  1. We decide and are up to speed what gets in xugrid
  2. This use case is not common, users who want to do this need to read the xugrid documentation anyway to see what they are doing, and therefore are informed about potential changes.
  3. Users providing objects directly can use their IDEs to find what are the current regridder objects in xugrid.

UPDATE: After some discussion, it appeared the lack of an intermediate layer between iMOD Python and xugrid didn't sit well with developers. @Manangka came up with the idea to use enmerators. I think this is the best compromise:

# In iMOD Python module, e.g. regridder_utils.py
import imod
from enum import Enum
import xugrid as xu

class RegridderType(Enum):
    CENTROIDLOCATOR = xu.CentroidLocatorRegridder
    BARYCENTRIC = xu.BarycentricInterpolator
    OVERLAP = xu.OverlapRegridder

# In user script
_regrid_options = {"var1": (RegridderType.OVERLAP, "mean")}
regridder_instances = {}

for name, (cls, method) in _regrid_options:
    regridder_instances[name] = cls(src_grid, target_grid, method)

This also has the advantage that users who care less about breaking changes, can use the xugrid objects directly as well. Because RegridderType.OVERLAP and xugrid.OverlapRegridder point to the same thing. For example, when xugrid gets a new regridder type, but iMOD Python is not up to speed to add it to its enumerator (or user hasn't updated iMOD Python yet, but did update xugrid). Furthermore, IDEs also hint at symbolic names in enumerators, so that helps with typos.

Manangka commented 10 months ago

In GitLab by @JoerivanEngelen on Jun 2, 2023, 11:37

After some discussion: the lack of a small interface between what the user provides and xugrid didn't sit well with some developers. @Manangka came up with the idea to use Enumerators. This I think is the best compromise between reducing code redundance and user friendliness.


# In iMOD Python module, e.g. in regridder_utils.py
import xugrid as xu
from enum import Enum

class RegridderType(Enum):
    CENTROIDLOCATOR = xu.CentroidLocatorRegridder
    BARYCENTRIC = xu.BarycentricInterpolator
    OVERLAP = xu.OverlapRegridder

# User script
_regrid_options = {"var1": (RegridderType.OVERLAP, "mean")}
regridder_instances = {}

for name, (cls, method) in _regrid_options:
    regridder_instances[name] = cls(src_grid, target_grid, method)

This has the advantage that users who care less about breaking changes in xugrid, can always use xugrid directly (as in the example above), as RegridderType.OVERLAP and xu.OverlapRegridder point to the same thing (unless xugrid makes a breaking change).