Opportunistic refactoring

[deltamarnix]

We will need to make quite some adjustments in the FloPy code base. We will need to refactor the current code base and split out certain functionality into a more modular approach, before we can actually implement some of the new ideas that we have. This discussion aims to make a breakdown of the work that is still to be done. From there we can start estimating the work.

Work with minimal impact on the API

Refactor PackageContainer to composition over inheritance ✅

I have been playing with the idea of doing a refactor and keeping as much in place as possible. I agree that PackageContainer and PackageInterface are daunting classes, but we can separate the functionality in a more modular fashion to simplify our intentions.

What we see in the current flopy design, is the overuse of inheritance where composition would most likely be sufficient. We see that both simulation, model, and package are inheriting from PackageContainer, while having a PackageContainer as a field would be sufficient. Inheritance should be used when there is also functionality that expects any kind of PackageContainer, no matter what the subclass is. I have not found such code. In this case inheritance was used to share functionality among different types of classes. Which can also be achieved with composition, as the PackageContainer doesn't need anything from its subclass and there are no abstract methods in PackageContainer.

During modflowpy/flopy#2324, I came to the point that it was very well possible to do this refactor, but I have doubts on which functions can actually be removed, because they are supposed to be for internal use only. There are options to either just remove them, keep them, or @deprecate them.

Move functions out of PackageInterface that can be done modular

We could also look at other types of functions that could move out, like the export functionality. It would make more sense to do this in a similar fashion to cattrs with a module and single-dispatch functions. The functions that can go out of PackageInterface are: plot(), export(), and check().

It is possible to do this with minimal user impact by keeping the plot() function, but @deprecate them in favor of the modular version. The plot() function in PackageInterface would simply point to the modular version.

Add type hints to critical public classes

I do expect that refactoring is a tough job at the moment, because there are no type hints in the code. VS Code cannot follow the methods and properties when renaming or moving and doesn't give any error messages when functions no longer exist. I recommend adding type hints in the classes we want to refactor, and let mypy run over it.

This can be done with minimal impact on backwards compatibility, as we are just adding type hints, and not removing anything for the user.

Move static methods to modules

Some functions are now decorated as @staticmethod, but they don't really apply to their class and could just as well be a global function in a module. E.g. Package.add_to_dtype() doesn't seem to use anything from the Package class and could just as well be a separate function.

Similar to making functions like plot() more modular, the same accounts for these @staticmethods. We could keep the old ones for now and @deprecate them. Then simply point to the new modular version.

Let generate_classes.py use jinja ✅

The code that is now generated with print statements and write() functions. It would be so much easier to use jinja, so it is clearer to read the initial file that is being used for generation. The jinja package handles template generation better than write() with raw strings.

Generate dfn classes on user install

See #35.

This idea would have minimal impact on the user, as it is being performed during get-modflow. So no changes in the API.

Work with likely more impact on the user

Use XArray / Dask to speed up performance

See #49.

Use attrs for property generation

There is quite some boilerplate code for variable getters and setters. The attrs package could help with this and simplify the code.

Introducing attr itself and simply replacing the things that we have would not have any impact on the user's code, but if we decide to move / remove certain variables, it will.

[wpbonelli]

a few more:

• consolidate input specifications — i.e. is everything in mfstructure.py necessary? is it necessary to reproduce the DFN spec verbatim as input component classes currently do? and similar questions
• investigate discretization module... can it be cleanly "unplugged" from existing core and plugged into a new framework?
• find the line between internal and user-facing APIs (this may not be well-defined at the moment) and begin to separate these from one another where appropriate/helpful
• investigate error handling approaches... why are flopy errors currently so uninformative? what can be done in the near-term to improve them? this will help refactoring efforts
• investigate the model check mechanism... correctness? extensibility?

[wpbonelli]

after some discussions I think we can outline a plan for checks:

• deprecate current check mechanism now (3.9) and unhook it in a later minor release (3.10?)
• tentatively leave the check API in place, just warn it will become non-functional for the rest of 3.x until we..
• reimplement checks for flopy 4

[wpbonelli]

the existing grid modules are stable and probably outside the scope of this work, but we should devote some attention to synchronizing them with dis and other packages

[wpbonelli]

maybe we should consider deprecating is_valid() methods like we have done for the check mechanism... what is the difference between these?

[deltamarnix]

Coming back to MFStructure. Our internal usages mainly look at sim_struct, flopy_dict, and model_types. It seems like we could generate this information during code generation because it is static information, and remove MFStructure in its entirety.

[wpbonelli]

It seems like we could generate this information during code generation because it is static information, and remove MFStructure in its entirety.

Excellent!

[deltamarnix]

I was playing a bit with "Move static methods to modules". I figured that the PackageContainer now has two functions. One for containing a list of all types of models, and one for containing a list of all types of packages. When we split this up, we see that modflow_models is actually never used and can go. And then we are only left with a meager dict models_by_type. I would move that to a very simple module like:

# _model_factory.py with an underscore so it's internal

_models_by_type: dict[str, type["MFModel"]] = {}

def register_model_type(model_type: str, model_class: type["MFModel"]) -> None:
    _models_by_type[model_type] = model_class

def get_model_class(model_type: str) -> type["MFModel"] | None:
    _models_by_type.get(model_type)

I don't think it needs to be anything more, and it can be internal this way. We can do a similar setup for the parts of package, because that does exactly the same.