moose
moose copied to clipboard
idaholab
Add connection to the Saline interface to MSTDB-TP
This adds a new fluid property class extension that wraps the Saline code, which is an interface to the MSTDB-TP salt fluid property databse. Saline is added as an optional submodule.
closes #22724
Reason
Design
Impact
I'll be on leave for the next two weeks, but @logan or @GiudGiud should be able to help you. I would prefer it if we used the same submodule approach (layout and make files) as we do in the thermochimica PR.
@dschwen was looking at multi-level includes (including Saline in FP, and running another module that builds with FP)
Let's squash these commits together a bit, especially the "update some file" commits.
Where are we on the documentation failure with thermochimica? Do we still need all recipes to check it out? If so, is that going to be ok for saline too?
Job Documentation on 74fe3d9 wanted to post the following:
View the site here
This comment will be updated on new commits.
The build is also failing because we are passing some strict flags from MOOSE (-Werror-unused etc) on Saline
Where are we on the documentation failure with thermochimica? Do we still need all recipes to check it out? If so, is that going to be ok for saline too?
That is fixed.
The build is also failing because we are passing some strict flags from MOOSE (-Werror-unused etc) on Saline
What a wonderful opportunity to fix the Saline code base :-D
The build is also failing because we are passing some strict flags from MOOSE (-Werror-unused etc) on Saline
What a wonderful opportunity to fix the Saline code base :-D
It's not about fixing anything here, we can't be imposing compilation flags to others. The MOOSE ecosystem is meant to be more inclusive than that.
EDIT: misclicked the close
A few options here:
- Saline gets modified to remove the warning
- #pragma GCC diagnostic ignored "-Woverloaded-virtual" get added to Saline
- we pass additional flags to undo the MOOSE ones
My preference would be to not request changes from the Saline team if that is a possible avenue.
My preference would be to not request changes from the Saline team if that is a possible avenue.
That was not the intention of my comment. I might make that pr to saline myself.
My preference would be to not request changes from the Saline team if that is a possible avenue.
That was not the intention of my comment. I might make that pr to saline myself.
Many of the Saline's compiler warnings have been addressed. There is an unused parameter which is reserved for pressure. This parameter may become required at a later date. If compiler options equivalent to GCC's combination of -Werror -Wall -Wextra then the equivalent of -Wno-unused-parameter will be required to compile Saline directly.
That's great news I just rebased and updated the submodule, see where we stand
We are still hitting the errors.
I just squashed unnecessary commits and tried adding another flag to build Saline. This may not be quite what we want, I ll have to check how far this flag propagates
something like : function(other arguments..... , Real /*pressure*/) will do here
It will remove the unused variable warning, while keeping the argument / same function signature
ok we cant do what I did here, it propagates to the whole repo
Daniel's fix wouldnt be too bad to do on the Saline side
Job Coverage on 74fe3d9 wanted to post the following:
Framework coverage
Coverage did not change
Modules coverage
Fluid properties
| 19aa8b | #22725 74fe3d | ||||
|---|---|---|---|---|---|
| Total | Total | +/- | New | ||
| Rate | 85.23% | 85.47% | +0.24% | 92.86% | |
| Hits | 7190 | 7258 | +68 | 52 | |
| Misses | 1246 | 1234 | -12 | 4 | |
Full coverage reports
Reports
-
framework -
chemical_reactions -
combined -
contact -
electromagnetics -
external_petsc_solver -
fluid_properties -
fsi -
functional_expansion_tools -
geochemistry -
heat_transfer -
level_set -
misc -
navier_stokes -
optimization -
peridynamics -
phase_field -
porous_flow -
ray_tracing -
rdg -
reactor -
richards -
scalar_transport -
solid_properties -
stochastic_tools -
tensor_mechanics -
thermal_hydraulics -
xfem
This comment will be updated on new commits.
Yes that will suppress the warning, but I am not sure that Saline intends to suppress the warning at this time. Either way I believe that is a different philosophical discussion and the problem at hand is MOOSE's use of the perfectly valid form of Saline which is already published.
Make provides for target specific variables, so the following may work:
saline : ADDITIONAL_CPPFLAGS += -DSALINE_ENABLED -Wno-unused-parameter.
I am not really proficient in Make and am definitely not versed in MOOSE's flavoring of Make. So the above may need some massaging.
I agree let's figure this make command out to enable the flag very locally to Saline
In the meantime a few things to consider:
- line coverage is very low. The way we get near 100% coverage in fluid properties is with a unit test. It makes it very easy to call each routine and check their output. See anything in fluid_properties/unit/src
- the current set of functions implemented likely only suffices for SAM, NSFV will want more AD routines, subchannel will want more some p_rho variable set routines, THM will want some (v,e) routines. While we dont need to do this now, I m raising awareness so we can have a plan
There is no saline target though in the Makefile. I think all the code is compiled under the same target. @permcody any idea how to move forward here
Just added a unit test and rebased due to a conflict with the submodule list (wasp just got added)
It seems there are two comments from Daniel left. If my PR gets accepted on the Saline side that will take care of the compile warnings
Test suite is failing because documentation is missing. I ll catch the formatting things in another commit but someone more qualified should write the actual SalineFluidProperties documentation (in doc/content/source/fluidproperties)
Testing already includes Saline and is now green (failure is Griffin, unrelated) @dschwen if you could please review, I have too many commits here
Needs a reviewer, then there may be additional comments to address. @idaholab/moose-ccb
@rks171 thank you for addressing the comments. I think this is just about ready to go as far as I'm concerned. Appreciate the contribution! Would you be able to squash your most recent commits which address my review, into one or a couple commits? It's sometimes more useful for looking at history to have a few commits with meaningful messages
