Skip to content

Entropy now available from EOSPAC #507

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Entropy now available from EOSPAC #507

wants to merge 13 commits into from

Conversation

aematts
Copy link
Collaborator

@aematts aematts commented May 17, 2025

PR Summary

Connected the eospac entropy to EOSPAC.
Work in progress on new test case test_kpt_full_circle.

PR Checklist

  • [ x] Adds a test for any bugs fixed. Adds tests for new features.
  • [ x] Format your changes by using the make format command after configuring with cmake.
  • [x ] Document any new features, update documentation for changes made.
  • [ x] 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.
  • LANL employees: make sure tests pass both on the github CI and on the Darwin CI

If preparing for a new release, in addition please check the following:

  • Update the version in cmake.
  • Move the changes in the CHANGELOG.md file under a new header for the new release, and reset the categories.
  • Ensure that any when='@main' dependencies are updated to the release version in the package.py

aematts and others added 7 commits February 6, 2025 15:50
* Update doc/sphinx/src/getting-started.rst

Co-authored-by: Christoph Junghans <[email protected]>

* update openmp reference

* builddir to build

* build -> builddir as easier to distinguish names. Remove the unit tests from getting started.

* Add references heading at bottom of paper

* prepare for release 1.9.1

* fix dois

* Thread through quadratic logs. Thanks Peter and Jacob.

* bounds typo

* output on failure

* useless error messages... and it works on my machine...

* formatting

* fix error bounds on CI machine

* clean up shared constants

* move to constexpr if with settings namespace

* it passes on my machine???

* the anchor appears shifted by 1 point, weirdly.

* or not supported...

* add eos_grid example

* remove eps which is unused

* move compute bulk modulus to BEFORE reinterpolating to fast logs

* formatting

* mantiss -> mantissa

* add versioning to sp5 files

* add ability to densify rho/T in fast log table for stellar collapse

* BMOD IS LOGGED

* zero-initialize sieOffset

* Add device macros to fix eospac builds on HIP and also fix evaluate for tables on CUDA

* changelog

* how did these get through previous tests

* Split evaluate

* const for device-side var. Pass by reference allowed for host side, if user is cautious.

* const correctness

* ci: update common.yml and add missing jobs

* Correct name to MixParams

* Need to initialize the struct

* Clang format

* fortran: add intrinsic to iso_c_binding

* fix typo

* ci: re-enable hard failures

* timestamp

* fixing EOSPAC preprocessor macros

* danl fixes. fix CI issue

* Update doc/sphinx/src/using-eos.rst

Co-authored-by: Jeff Peterson <[email protected]>

* remove all SG_PIF_NOWARNS

* portable always abort

* add include guard for kokkos kernels in find

* add more time for eos tabulated tests

* run singularity tests through clang warnings, clang address sanitizer, clang undefined behavior sanitizer

* sneak in kokkos update in submodules

* clean up some more warnings clang found

* add ability to build fortran backend code without fortran. Useful for clang-sanitize builds, since flang is not super well supported yet

* enable_testing was missing... how did tests ever run????

* add sanitizer test

* rename sanitizer

* Add new build flags

* correction

* changelog

* add check for warnings on gcc as well as clang

* switch to [[maybe_unused]] because were on C++17

* start adding mean atomic mass and number

* Rela -> Real

* dont shadow T parameter

* Add MeanAtomicProperties for analytic EOS's to use and implement for ideal gas

* test

* formatting. Add carnahan starling AZbar

* eos davis

* more descriptive comment

* Move default implementations of MeanAtomicMass/MeanAtomicNumber to macro

* spiner already had Abar and Zbar in metadata. Just add it to class.

* temperature -> T

* CHANGELOG

* docs

* docs for tables

* minor cleanup of comments/error messages

* add mean atomic mass/number test for tabulated

* AZbar_ in EOSPAC, not _AZbar

* printparams should be host device

* Atomic mass and number

* ideal gas electron EOS implemented. Still need to add zsplit and documentation

* zsplit mostly implemented

* zsplit compiles

* tests work

* add test for zsplits

* messages

* Add discussion of lambdas, add discussion of 3T, add discussion of ZSplit and IdealElectron modles

* changelog

* clang format 12 stay around please

* latest for docs, noble for formatting

* try updating to latest catch2 release

* unused variable

* typo and also try to make wno-psabi only for C++

* try to fix cuda errors for static members

* No GCC. You are wrong and I am right.

* disable some EOSs from default variant

* shrink the default variant

* Disable python build in github tests as it now takes an incredibly long time, for unclear reasons

* update build to be parallel

* try enabling python build now again with parallel builds

* undo removal of relativistic EOS

* fix type list comma

* add parallel builds for the docs build

* WHY IS IT DYING WITHOUT AN ERROR MESSAGE

* turn off IPO to see if this fixes the memory problems with the linker

* no need for verbose output

* switch to ld.gold linker

* huh...

* disabling intraprocedural optimization seems to way slow down the build

* extend memory limits on system, use release build to remove debug symbols

* runner not available for ubuntu-noble now. Try older version string?

* add doc info

* add v&v EOS option to spackage

* spackage should have vandv set to true

* document spackage change

* Sam jones comments

* Update doc/sphinx/src/modifiers.rst

Co-authored-by: Jeff Peterson <[email protected]>

* Update doc/sphinx/src/modifiers.rst

Co-authored-by: Jeff Peterson <[email protected]>

* hjhp and pdmullen comments

* ci: update settings

* spack updates

* add RhoE of PT to base class and implement where relevant

* formatting

* Tests pass on macbook. Still need to add a few tests

* swap default to findRoot because it respects bounds

* tests complete for everything except eospac and spiner

* update zsplit

* changelog

* missing auto

* it works on my machine...

* output on failure please

* just change tests to output-on-failure... and also loosen EPS in case thats the issue

* lets try this...

* make default implementation more robust maybe I hope

* put default rho gues close to STP

* put default rho gues close to STP

* re-tighten bounds after initial guess problem resolved

* oops missing boolean not

* starting to add in stuff Daniel wrote, piecewise so I can check it all compiles

* add jacobian

* add several comments based on Jeff's feedback

* try 1e-8 MINR_DEFAULT

* Make base class use MaximumDensity

* update docs

* typo

* ok... lets try this

* make tests more effectively check for accurate pressure

* residual

* formatting

* namespace

* tweak how to check this residual to work when P = 0

* oops need to do magnitudes

* come up with sensible bounds for power series EOSs.

* I give up. 1e-4 it is

* comment

* remove unused variable

* tighter tolerances

* relax tolerance for chekc

* remove the maximum densities... we're never going to hit those bounds anyway and I think they might be interacting badly with GPUs

* also add sanity check for rhomax vs rhomin

* missed leading underscore

* stupid C++ deleting copy constructor for const correctness

* choose worse, but more generic guess... does that help?

* is it because rho is uninitialized?

* update docs

* PTE space PTE solver compiles at least

* add to simplest test but it fails lol

* tweak initial guess

* add introspection for pressure

* It seems to work, at least tentatively

* docs

* changelog

* CC

* not the boolean I meant to express

* unused var

* oops... big typo

* add default nullptr argument

* change name

* max press AT temp

* add OMP_PROC_BIND for unit testing

* split scratch

* update test pte to split profiling

* spack: update package.py files

* ci: update build_and_test.sh

* spack updates

* move OMP_PROC_BIND option to BUILD AND TEST

* move OMP_PROC_BIND option to BUILD AND TEST

* Add multi ideal gas PTE test that checks for agreement between PT space and RT space solvers

* Fix residual to correctly check for mass fraction weighted energy

* remove databox dependency

* fix loops that cause warnings

* int to size_t

* more size_t conversions

* no ports-of-call-array

* fix formatting and typos

* put test pte ideal in generator expression

* make sure solver sets pressure and temperature and then gets new microphysical density and energy from that. otherwise initial guess can cause solver to report convergence

* control tolerances so densities match after fixing init issue

* test PTE infrastructure now supports arbitrary solver... though this isn't stressed at this time.

* oops forgot to set nsuccess

* Works with multiple kind of EOS

* update closure documentation with correct formulae

* Jacobian inversion analytic

* CC

* ok good enough

* spack updates

* Update doc/sphinx/src/using-closures.rst

Co-authored-by: Jeff Peterson <[email protected]>

* Update doc/sphinx/src/using-closures.rst

Co-authored-by: Jeff Peterson <[email protected]>

* heading levels

* update docs based on Jeff's comments

* more detailed performance discussion

* JHP comments on mixed_cell_models.hpp

* prepare for adding PT-space solver

* Add 3 state PTE test

* comment

* CC

* spack updates

* Fixed EOSPAC.cmake Install Directory

* Update spiner submodule, for new Databox::interpToReal overload.

* spack updates

---------

Co-authored-by: Jonah Miller <[email protected]>
Co-authored-by: Christoph Junghans <[email protected]>
Co-authored-by: Jonah Miller <[email protected]>
Co-authored-by: Jonah Miller <[email protected]>
Co-authored-by: Richard Berger <[email protected]>
Co-authored-by: Jeffrey H Peterson <[email protected]>
Co-authored-by: Jeff Peterson <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Ben Bergen <[email protected]>
Co-authored-by: Ryan Thomas Wollaeger <[email protected]>
* merge main

* remove extra notes

---------

Co-authored-by: Jonah Miller <[email protected]>
Preparing for next work session
…rial_xxxx

files, and the specific input and output to compare to are in pte_test_xxxx.
Finally test_pte_{2,3}xxxx files are identical, except for commenting different things out.
…_full_circle, past Gibbs Free energy test.
@aematts aematts requested review from jonahm-LANL and jhp-lanl May 17, 2025 00:59
@aematts
Copy link
Collaborator Author

aematts commented May 17, 2025

I did not check that it ran on Darwin CI, but I worked on Darwin, so it should be fine.

@jhp-lanl
Copy link
Collaborator

I did not check that it ran on Darwin CI, but I worked on Darwin, so it should be fine.

I'll run the Darwin CI for you and make sure

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! I think some commented code needs to be removed and then this is ready.

@@ -184,6 +188,7 @@ EOS_INTEGER eosSafeLoad(int ntables, int matid, EOS_INTEGER tableType[],

// choice of which table types setOption is called on mimics SAP. Some table types are
// incmopatible whiel others lead to numerical issues.
// aem: should EOS_St_DT go in here?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean that the entropy table maybe shouldn't have some of these options?

I would say it should be skipped for the monotonicity option (EOS_Ut_DT should as well`).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel that you or Jonah need to decide that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 I second @jhp-lanl we should put EOS_St_DT in the list of table types that should be excluded on this line:

      if (tableType[i] != EOS_Uc_D and tableType[i] != EOS_Ut_DT and
          tableType[i] != EOS_Ut_DPt) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aematts If you'd prefer, I can make that change in a separate MR.

Comment on lines +31 to +36
#include <pte_test_3phaseSesameSn.hpp>
//#include <pte_test_2phaseVinetSn.hpp>

using namespace pte_test_3phaseSesameSn;
// using namespace pte_longtest_2phase;
// using namespace pte_test_2phase;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete commented code?

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.

Thanks for this @aematts ! Nice work! Do you think we can create an isolated merge request just for the entropy stuff, that we can merge first? Then we can update your branch? I can try to do it if you're not comfortable doing it through git.

@@ -184,6 +188,7 @@ EOS_INTEGER eosSafeLoad(int ntables, int matid, EOS_INTEGER tableType[],

// choice of which table types setOption is called on mimics SAP. Some table types are
// incmopatible whiel others lead to numerical issues.
Copy link
Collaborator

Choose a reason for hiding this comment

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

My typo sorry...

Suggested change
// incmopatible whiel others lead to numerical issues.
// incompatible while others lead to numerical issues.

@@ -184,6 +188,7 @@ EOS_INTEGER eosSafeLoad(int ntables, int matid, EOS_INTEGER tableType[],

// choice of which table types setOption is called on mimics SAP. Some table types are
// incmopatible whiel others lead to numerical issues.
// aem: should EOS_St_DT go in here?
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 I second @jhp-lanl we should put EOS_St_DT in the list of table types that should be excluded on this line:

      if (tableType[i] != EOS_Uc_D and tableType[i] != EOS_Ut_DT and
          tableType[i] != EOS_Ut_DPt) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want this to be performant, we also need to add these two overloads.

  // general template
  template <typename RealIndexer, typename ConstRealIndexer, typename LambdaIndexer>
  inline void EntropyFromDensityTemperature(ConstRealIndexer &&rhos,
                                                   ConstRealIndexer &&temperatures,
                                                   RealIndexer &&ss, const int num,
                                                   LambdaIndexer &&lambdas) const {
    EosBase<EOSPAC>::EntropyFromDensityTemperature(rhos, temperatures, ss, num, lambdas);
  }    

  // specialization
  template <typename LambdaIndexer>
  inline void
  EntropyFromDensityTemperature(const Real *rhos, const Real *temperatures,
                                       Real *ss, Real *scratch, const int num,
                                       LambdaIndexer /*lambdas*/,
                                       Transform &&transform = Transform()) const {
    static auto const name =
        singularity::mfuncname::member_func_name(typeid(EOSPAC).name(), __func__);
    static auto const cname = name.c_str();
    using namespace EospacWrapper;
    EOS_REAL *R = const_cast<EOS_REAL *>(&rhos[0]);
    EOS_REAL *T = const_cast<EOS_REAL *>(&temperatures[0]);
    EOS_REAL *S = &ss[0];
    EOS_REAL *DSDT = scratch + 0 * num;
    EOS_REAL *DSDR = scratch + 1 * num;

    EOS_INTEGER table = SofRT_table_;
    EOS_INTEGER options[3];
    EOS_REAL values[3];
    EOS_INTEGER nopts = 0;

    // Set up density/temperature unit scaling
    impl_eospac::SetUpDensityTemperatureScalingOptions(options, values, nopts, transform);

    // Entropy units are u / T. T is the same, so transformation proportional to u
    impl_eospac::SetUpOutputScalingOption(options, values, nopts, transform,
                                          pressureFromSesame(1.0));

    eosSafeInterpolate(&table, num, R, T, S, DSDR, DSDT, "SofRT", Verbosity::Quiet,
                       options, values, nopts);
  }

Some formatting probably necessary

Copy link
Collaborator

@jhp-lanl jhp-lanl May 19, 2025

Choose a reason for hiding this comment

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

I was thinking about that as well, but I think the overloads can be a separate MR if needed.

EDIT - @aematts let us know if you'd prefer this be part of a second MR

@aematts
Copy link
Collaborator Author

aematts commented May 19, 2025

Thanks for this @aematts ! Nice work! Do you think we can create an isolated merge request just for the entropy stuff, that we can merge first? Then we can update your branch? I can try to do it if you're not comfortable doing it through git.

I had a very hard time last to merge what I had left with all the work you and Jeff had done. I would prefer to just put all of it in. The pte tests I have refactored are as want them, I just cannot answer Jeff's question without going through it carefully and I do not have time just now. (all my training is overdue :(.)
Once full circle is done we might even take away some of the other tests. But I do not know about that until I have finished. The last part of full circle just now is used as a test for entropy and Gibbs.

@Yurlungur
Copy link
Collaborator

Ok---now that we've given feedback, would you prefer to keep working on it and merge when you're done? Or is this a "unit" of work that can be merged?

@aematts
Copy link
Collaborator Author

aematts commented May 19, 2025

Remember I wanted you two to fine tune the entropy. In particular in the tests you have using 5030 (air) that doesn't have an entropy table (nor a Helmholtz free energy table). So go ahead and change what you need there.

@aematts
Copy link
Collaborator Author

aematts commented May 19, 2025

Ok---now that we've given feedback, would you prefer to keep working on it and merge when you're done? Or is this a "unit" of work that can be merged?

I think this is a unit of work that can be merged. What I do need to do more is finishing up full circle with mass fraction updates, but that is well contained and not dependent on the rest.

@aematts
Copy link
Collaborator Author

aematts commented May 19, 2025

This is what I told Jeff by email, I seem to have forgotten to copy you Jonah:
It is probably safest to only load Pt_DT and Ut_DT first and then do a trial load of St_DT that is aborted if the table doesn’t exist. But the user need to know, so an error message would be OK still. But not everytime you try to use eospac for something…

@Yurlungur
Copy link
Collaborator

Remember I wanted you two to fine tune the entropy. In particular in the tests you have using 5030 (air) that doesn't have an entropy table (nor a Helmholtz free energy table). So go ahead and change what you need there.

Ah ok. I think we still want to test that table, but we should also test a table that does have native entropy. 4272 maybe. Will look to see what's available.

@aematts
Copy link
Collaborator Author

aematts commented May 19, 2025

I have one more issue I am dealing with. I cannot use Catch when I am in a main routine. Or can I? I wanted to use array_compare but could not figure out how to catch the error... I use isClose now but that doesn't report errors automatically...

@jhp-lanl
Copy link
Collaborator

I have one more issue I am dealing with. I cannot use Catch when I am in a main routine. Or can I? I wanted to use array_compare but could not figure out how to catch the error... I use isClose now but that doesn't report errors automatically...

Catch needs its own main() routine, so that's probably the main issue.

If you want to use Catch, you'll need to do the following:

  1. Create a *.cpp file to perform the Catch/Kokkos initialization. This can be a copy of an existing one, but you'll include different test headers.
  2. Transform your *.cpp files into headers and instead of compiled files. Then instead of int main(...) in these files, you'll want to use either TEST_CASE(...) or SCENARIO(...) to define a test in much the same way as the main() functions currently work. The ... are two arguments where the first is a unique name and the second is a set of tags for the test (see other tests for examples).
  3. Use Catch assertions (https://github.com/catchorg/Catch2/blob/devel/docs/assertions.md) to check the values you want to check. I believe array_compare uses these under the hood.

If you have things you want to reuse between tests, you can use the SECTION macro. Everything in a SECTION is isolated from other sections (except contained sections), and the lowest level of SECTION is like a leaf in a tree. The test framework visits each leaf individually, within the scope of higher levels. You can see some examples here: https://github.com/catchorg/Catch2/blob/devel/docs/tutorial.md#top

Also, we generally use BDD (behavior driven development) style tests, but you probably don't have to as a first pass. The basic difference is that BDD tests are designed to function like sentences (e.g. "GIVEN a certain value, WHEN the value is passed to my function, THEN the result should be x." In this example GIVEN, WHEN, and THEN are all aliases for SECTION in my above description. Again, it's probably useful to look at our other tests for examples.

@Yurlungur
Copy link
Collaborator

@aematts I think testing this functionality should be a separate MR which I can pursue once we have this in. I want to add the entropy to the Spiner tables as well so that we can cross compare 4272 (Steel) the same way we do now but for entropy.

@aematts
Copy link
Collaborator Author

aematts commented Jun 5, 2025

@Yurlungur and @jhp-lanl, this fell of the radar... I wanted you to fix the problem with not all sesame tables having an entropy table. My implementation fails when that is the case, as for 5030. Since this has to do with how singularity-eos interacts with EOSPAC, my thinking was that this task was best left to experts...

@aematts
Copy link
Collaborator Author

aematts commented Jun 5, 2025

I will be on vacation 10-20 June but hope that I can start working on this again when I am back.

@Yurlungur
Copy link
Collaborator

@Yurlungur and @jhp-lanl, this fell of the radar... I wanted you to fix the problem with not all sesame tables having an entropy table. My implementation fails when that is the case, as for 5030. Since this has to do with how singularity-eos interacts with EOSPAC, my thinking was that this task was best left to experts...

I don't think we want to actually fix that--just to fail gracefully. At least for now.

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.

4 participants