modflow6
modflow6 copied to clipboard
refactor(DisBaseType): make abstract, use interfaces and deferred procs
Motivation
DisBaseType hosts shared logic and specifies the interface discretizations should implement, but does not fully define a discretization and so should not be instantiated. To reflect this, this PR makes it abstract, replacing stubs which raised not implemented errors with interfaces and deferred procedures.
Calling overridden procedures from children
A consequence here is that child types can no longer call the abstract parent's procedure if the child overrides it. The language standard allows routines in derived types extending an abstract parent to call non-overridden procedures but not overridden ones. For discussion see:
- https://fortran-lang.discourse.group/t/call-overridden-procedure-of-abstract-parent-type/590/1
- https://community.intel.com/t5/Intel-Fortran-Compiler/Calling-concrete-type-bound-procedures-on-abstarct-derived-types/m-p/941886
I think the standard could support this — explicitly identifying the parent keeps the binding unambiguous. This makes it sound like there is no technical obstacle, just a desire not to depart from existing semantics.
There are a few ways around it. This PR demos two:
- name the parent's version something else, e.g.
foo_default, rebind it to a generic name e.g.foo => foo_default(and likewise with the override in the child) so external callers can continue to use e.g.dis%foo(), and have children usethis%foo_default()— recommended in the discourse link, used forallocate_scalarsandallocate_arrays - make the procedure public in the parent module and call it via traditional
call foo(this)instead ofthis%foo(), as recommended in the Intel thread — used fordis_da
A third option (also in the discourse link) is to introduce a concrete grandparent (DisBaseSharedType?) for procedures whose implementation is shared by all children, extend it with an intermediate abstract parent containing deferred procs, have all the children extend the intermediate, and call from children with e.g. this%DisBaseSharedType%foo().
Whichever approach is deemed best, the PR can be updated to use consistently, if any of this is even worth it. Maybe it's not. But maybe there is a case for letting the compiler enforce constraints instead of runtime programmer errors.
At first I wondered if this is relevant to FlowModelInterfaceType too but that's a more complete abstraction and maybe there could be situations calling for its direct use without extension.
Miscellany
Also some minor edits in DiscretizationBase.f90.
- use compact docstrings
- remove unneeded
returnstatements - remove unneeded
publicmodifiers from type-bound procedure listings
The same could be done in dis/disv/disu but thought best to wait so the changes here are easier to see.
A few comments...
I agree that compile time errors would be preferable to see in general. I also agree that the language is ambiguous in this way- it isn't necessarily obvious to me which of these is the best pattern (or when it would be best to use one or the other) or if there are others.
I've thought through the same questions and have had to make some similar choices. For what it's worth, I did implement a version of the 3rd pattern you refer to in IDM. I made the choice in part because I thought it might be the most straightforward to absorb when looking at the code with the intention of extending it.
My initial reaction to the 2 exemplified patterns as a general practice is that I might prefer the first to the second, mostly because intuitively the objects are still object like and don't require a set of file scoped interfaces to get them to correctly behave as an hierarchy. That being said, I think the second pattern would be useful in situations where a related set of interfaces are defined that broadly apply across several or many modules- this might be how you have it set up here.
If we can reach some consensus on when it might be best to apply what approach, I would definitely be on board with using that approach with new development. It would probably be my preference to hold off on refactoring existing code until we go through that process a couple of times- that's just my take though.
Thanks @mjreno really helpful comments, it is not obvious to me which approach is best either — agreed on experimenting in new development before making changes
I think besides discretization this may be relevant in base model, boundary package, and advanced package transport, maybe I've missed other places
reopen if we determine worthwhile later on