search

TDycores-Project / TDycore

BSD 2-Clause "Simplified" License
4 stars 0 forks source link

Commit-by-commit port of Rosie's salinity PR to new code structure #240

Closed jeff-cohere closed 2 years ago

jeff-cohere commented 2 years ago

Hi everyone,

@leorosie and I were working on a lot of things at the same time, so our paths diverged. This PR is a reimagining of PR #239 in the new code structure in which material models, characteristic curves, etc are factored out, and our new "polymorphic" model is in place, with vectorized functions and all that wacky stuff.

I've revised each of Rosie's commits and stuck both of our names on them using a convention suggested by @jedbrown (thanks!). Hopefully the code is similar enough for us to evaluate it alongside PR #239 . I took the liberty of changing the names of a few things ("concentration" to "salinity" to be more specific, and "PorDiff" to "SalineDiffusivity", etc), but these are just suggestions.

It looks like there may be some test failures in the transient MPFAO F90 demos. Perhaps they result from the slight change in the "constant" density of water introduced in this PR.

jeff-cohere commented 2 years ago

Hey @leorosie ,

I don't know if you're back in front of your glowing screen, but if so, I hope you had a nice break!

Do you mind if I change the water density value and the boundary condition default option that @bishtgautam has mentioned above? And do you have thoughts on this PR overall (names, organization, etc)?

leorosie commented 2 years ago

@jeff-cohere The water density value and boundary condition default option were changed to match PFLOTRAN so those can both be changed back. The rest looks good to me, thanks for taking this on!

jeff-cohere commented 2 years ago

Okay, I think all this is ready. We don't have regression tests yet for salinity, though. @bishtgautam , do you want those to be added in a subsequent PR, or should we add them to this one before we merge?

bishtgautam commented 2 years ago

@jeff-cohere Let's add a new test in the PR itself. Thanks.

jeff-cohere commented 2 years ago

@jeff-cohere Let's add a new test in the PR itself. Thanks.

@leorosie , are you comfortable coming up with such a test (or borrowing one from PFLOTRAN)? I have a cartoon-level understanding of the equations, but I am no salinographer. :-)