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 CNC to generalized transport class #1404

Closed

Conversation

emorway-usgs
Copy link
Contributor

No description provided.

@emorway-usgs
Copy link
Contributor Author

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

Copy link
Contributor

@langevin-usgs langevin-usgs left a comment

Choose a reason for hiding this comment

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

Looks good, @emorway-usgs. We might want to think about renaming the TSP object to something other than CNC since this the second C refers to concentration. Maybe it could be something like SDV (specified dependent variable)? But we would still want the user to specify CNC for transport, and maybe something else for temperature?

src/Model/TransportModel/tsp1cnc1.f90 Outdated Show resolved Hide resolved
@emorway-usgs
Copy link
Contributor Author

We might want to think about renaming the TSP object to something other than CNC ...maybe it could be something like SDV (specified dependent variable)? But we would still want the user to specify CNC for transport, and maybe something else for temperature?

@langevin-usgs, I like SDV and will plan to implement that as part of this PR.

Regarding a potential alternative name for the GWE equivalent of CNC, what about CTP (constant temperature), or would it be better to simply use CNT (only change the last letter)?

@emorway-usgs emorway-usgs force-pushed the ci-diagnose_elv_cnc_2_tsp branch from 541e356 to fd031dd Compare October 17, 2023 14:20
@emorway-usgs emorway-usgs marked this pull request as draft October 17, 2023 18:24
@emorway-usgs emorway-usgs force-pushed the ci-diagnose_elv_cnc_2_tsp branch from d3a3e97 to a5a5ad2 Compare October 17, 2023 19:26
@emorway-usgs emorway-usgs marked this pull request as ready for review October 17, 2023 21:16
Copy link
Contributor

@langevin-usgs langevin-usgs left a comment

Choose a reason for hiding this comment

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

This looks good, but I wonder how this is going to play with the new Input Data Model (IDM) routines. We are still going to want separate DFN files for CNC and CNT, but I'm not sure how this is going to work IDM. Perhaps you should look at the constant head package for flow and see if you can implement IDM for CNC as part of this PR. Since IDM is fully implemented now for CHD, WEL, DRN, RIV, GHB, EVT, and RCH, it would make sense it is implemented now for CNC as well. Any thoughts?

@emorway-usgs
Copy link
Contributor Author

@langevin-usgs, Thanks for bringing the IDM issue to my attention. Agreed on the separate .dfn files for CNC and CNT. From a documentation (mf6io.pdf) perspective alone it makes sense to have 2 separate .dfn files.

Also agree that it makes sense to look into IDM support for SDV now. I'll have a look at what was done to CHD for supporting IDM and may need to get together with @mjreno for the finer points.

@langevin-usgs langevin-usgs marked this pull request as draft October 23, 2023 13:10
@emorway-usgs
Copy link
Contributor Author

Going in a different direction with CNC. Going to leave it as part of the GroundWaterTransport model for the time being. A parallel package, constant temperature ("CNT"), will be introduced with GWE. After CNT is introduced, can look into creating a new class that contains the common code among CHD, CNC, & CNT that each of these packages could inherit from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants