Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(DisBaseType): make abstract, use interfaces and deferred procs #1417

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

wpbonelli
Copy link
Contributor

@wpbonelli wpbonelli commented Oct 31, 2023

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:

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 use this%foo_default() — recommended in the discourse link, used for allocate_scalars and allocate_arrays
  • make the procedure public in the parent module and call it via traditional call foo(this) instead of this%foo(), as recommended in the Intel thread — used for dis_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 return statements
  • 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.

@mjreno
Copy link
Contributor

mjreno commented Nov 6, 2023

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.

@wpbonelli
Copy link
Contributor Author

wpbonelli commented Nov 6, 2023

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

@wpbonelli wpbonelli added code refactor Nonfunctional changes maintenance Sprucing up the code labels Dec 11, 2023
@wpbonelli wpbonelli added the onhold Waiting for something label Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code refactor Nonfunctional changes maintenance Sprucing up the code onhold Waiting for something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants