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

Bjm/stiffgas #274

Merged
merged 27 commits into from
Jul 27, 2023
Merged

Bjm/stiffgas #274

merged 27 commits into from
Jul 27, 2023

Conversation

c0sm0-kramer
Copy link
Collaborator

@c0sm0-kramer c0sm0-kramer commented Jul 12, 2023

PR Summary

Added Stiffened Gas EoS according to reference Élaboration des lois d'état d'un liquide et de sa vapeur pour les modèles d'écoulements diphasiques, International Journal of Thermal Sciences, O Le Métayer and J Massoni and R Saurel, https://doi.org/10.1016/j.ijthermalsci.2003.09.002. Built with cmake 3.26.4 and make 3.82, compiled and tested with gcc 10.3.0.

This allows for the use of a stiffened gas EoS.

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file
  • If preparing for a new release, update the version in cmake.

Benjamin Joel Musick added 4 commits July 11, 2023 15:13
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good! Thanks for adding this. A few minor nitpicks below. In addition, some bigger picture things:

  1. We should test this on GPUs. the Darwin CI pipeline will do that for us, but if you have access to a GPU machine it's worth trying to run the tests there yourself too.
  2. We try to add the PR to the CHANGELOG.md file
  3. It would be good to add documentation about this EOS to the models.rst file in doc/sphinx/src. You can see how the others there are done. It doesn't have to be very extensive.
  4. Looks like your code isn't formatted. We use clang-format to automatically format our code. If you have clang-format installed, you can run it on singularity-eos automatically in one of two ways: first, you can type make format_singularity-eos in your build directory. Alternatively, if that isn't working for you, there's a script in utils/scripts called format.sh. You can run it to format your code. I recommend running it as:
FMT=path/to/clang-format VERBOSE=1 utils/scripts/format.sh

where FMT points to the path of your clang-format executable.

singularity-eos/eos/eos.hpp Show resolved Hide resolved
singularity-eos/eos/eos_stiff.hpp Outdated Show resolved Hide resolved
singularity-eos/eos/eos_stiff.hpp Outdated Show resolved Hide resolved
singularity-eos/eos/eos_stiff.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@jhp-lanl jhp-lanl left a comment

Choose a reason for hiding this comment

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

Good start Ben!

We should decide how to do entropy. I'd prefer this to match the ideal gas case, but I'm open to arguments as to why this is better. Numerically, I think the ideal gas case is simpler though.

I'd like to see a few sanity tests like:

  • Can you create an ideal gas EOS and a stiffened gas EOS with appropriate parameters and have them return the same result?
  • Are the asymptotic limits correct? You'll need to address some of the numerical issues above (divide by zero, etc.), but for example $P(\rho, 0)$ or $P(0, T)$ should return $-P_\infty$.
  • Check the bulk modulus against a finite difference of the pressure (see the Gruneisen EOS for an example)
  • Use a finite difference to approximate $T\left(\frac{\partial S}{\partial T}\right)_V$ and see if that's equal to the constant heat capacity.

EDIT - And of course add documentation for the new model.

singularity-eos/eos/eos_stiff.hpp Outdated Show resolved Hide resolved
singularity-eos/eos/eos.hpp Show resolved Hide resolved
singularity-eos/eos/eos_stiff.hpp Outdated Show resolved Hide resolved
singularity-eos/eos/eos_stiff.hpp Outdated Show resolved Hide resolved
singularity-eos/eos/eos_stiff.hpp Outdated Show resolved Hide resolved
singularity-eos/eos/eos_stiff.hpp Outdated Show resolved Hide resolved
singularity-eos/eos/eos_stiff.hpp Show resolved Hide resolved
singularity-eos/eos/eos_stiff.hpp Outdated Show resolved Hide resolved
singularity-eos/eos/eos_stiff.hpp Outdated Show resolved Hide resolved
singularity-eos/eos/eos_stiff.hpp Outdated Show resolved Hide resolved
Benjamin Joel Musick added 7 commits July 13, 2023 12:41
and adds robust::SMALL() to logorithms to prevent potential numerical issues

Not fixed/touched:
Appropriate floor for entropy, energy, pressure
Entropy formulation
Bulk modulus formulation
Daniel's optimizations
version 1 inputs : gm1, Cv, Pinf, q
version 2 inputs : gm1, Cv, Pinf, q, q', T0, P0
@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Jul 25, 2023

@c0sm0-kramer after you add documentation I think this is good to go.

EDIT - Actually can you add a couple more tests. I'll make a new review

test/test_eos_stiff.cpp Show resolved Hide resolved
Copy link
Collaborator

@jhp-lanl jhp-lanl left a comment

Choose a reason for hiding this comment

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

Looks like it's just about there. A few more changes and it should be good to go.

Ping me on mattermost when you make the changes and I'll run the CI on gitlab and get this merged when that passes

test/test_eos_stiff.cpp Outdated Show resolved Hide resolved
test/test_eos_stiff.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@jhp-lanl jhp-lanl 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 to me! I'll run the CI on gitlab and then if that passes we just need @Yurlungur to approve and this is good to go. Thanks a lot @c0sm0-kramer !

@jhp-lanl
Copy link
Collaborator

CI failed on GPUs because of missing i declaration. I've moved the declaration of i into the loops in the test so that it's more limited in scope.

@c0sm0-kramer all we need is documentation and this is good to go. Please add the EOS to the models section of the documentation.

Copy link
Collaborator

@jhp-lanl jhp-lanl left a comment

Choose a reason for hiding this comment

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

Small nitpick: can you move the stiff gas EOS model up just after the ideal gas model in the documentation rather than at the end? Otherwise this looks good!

doc/sphinx/src/models.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@Yurlungur Yurlungur 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 very good! Thanks for all your effort in pulling this through.

@Yurlungur Yurlungur merged commit 1d884ef into main Jul 27, 2023
4 checks passed
@Yurlungur Yurlungur deleted the bjm/stiffgas branch July 27, 2023 21:06
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.

5 participants