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(tsp): Elevate APT to generalized transport class #1428

Merged

Conversation

emorway-usgs
Copy link
Contributor

Relates to:
#1386
#1391
#1392
#1396
#1398
#1399
#1400

  • Includes updates in SFT, LKT, MWT, & UZT owing to the changes in APT
  • Ran fprettify
  • Updated meson
  • Ran build_makefiles

Copy link
Contributor

@aprovost-usgs aprovost-usgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a quick look through all the files, and though there are multiple files involved, overall the changes seem straightforward and appropriate. There is some coding and some inline comments that specifically reference gwe/energy/temperature, which maybe goes a little beyond the minimum necessary to elevate gwt for now, but I don't see much harm in that. I assume many of the inline comments are temporary, and you'll scan for those later and delete them or convert them to "real" comments before release.

Comment on lines +80 to +81
integer(I4B), pointer :: idxprepak => null() !< budget-object index that precedes package-specific budget objects
integer(I4B), pointer :: idxlastpak => null() !< budget-object index of last package-specific budget object
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear why we need these.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are part of a modification by @aprovost-usgs for the benefit of UZE. In formulating the matrix problem, the UZE balance equation is folded into the corresponding cell balance equation. In the subsequent budget calculations, the heat exchanged between the UZE feature and the cell (by virtue of their coming to thermal equilibrium) is "backed out" as the residual of the UZE budget terms. Therefore, the UZE-package-specific budget calculations, which include this residual calculation as the final step, need to come after the generic, APT-based budget calculations. Although (currently) only UZE should require this reordering, it is simplest to do it that way for all advanced packages and should pose no problem. idxprepak and idxlastpak are indices that are maintained so that the package specific calculations can be initially skipped and then done at the end.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, UZE could have overridden parent routines and stored these indices on its own. I'm fine with whatever way you want to go, especially if you think this is general and possibly useful beyond UZE in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@langevin-usgs Agree there are several ways this could be done. The bottom-line requirement is that all budget terms be in place before UZE calculates its residual.

-- The way it's done now, duplication of code is minimized, but it's admittedly a bit klugey to skip the package-specific calls and then comeback to them at the end.

-- Riffing off of the way it's done now, the kluginess could be eliminated by maybe reordering the indexing of the budget terms in APT so the package-specific terms come last. Haven't thought through the possible side effects.

-- As you suggested, UZE could override and handle all the budget calculations, doing them in whatever order it needs, at the expense of some code duplication.

-- Could break out the residual calculation as a separate step at the end. This is actually a more generally applicable approach, since the alternatives above will work only as long as UZE is the only APT package that needs to calculate its residual. So the idea would be to complete (in any order) all the generic and package-specific budget calculations other than the residuals, then at the end call package-specific residual-calculating routines (which would do nothing for most packages). Not sure if we'll ever have another residual-dependent package like UZE, but maybe it would be best to go ahead set it up that way anyway? The cost would be an additional subroutine baked into the APT infrastructure. (It's been a while since I looked deeply at this part of the code, but I believe it would be ok to have multiple residual-dependent packages, since the residual represents an "internal" transfer within each package and shouldn't affect other packages.)

Happy to do it whatever way is best for MF6 in the long run.

emorway-usgs added a commit to geohackweek/modflow6-tempfork that referenced this pull request Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code refactor Nonfunctional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants