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

Helmholtz EOS #264

Merged
merged 72 commits into from
Aug 25, 2023
Merged

Helmholtz EOS #264

merged 72 commits into from
Aug 25, 2023

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Jun 20, 2023

PR Summary

Preliminary work on implementing a Helmholtz EOS in singularity-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.

@jhp-lanl
Copy link
Collaborator

I'm sure with a paper and some time I could decipher much of this, but as it stands I'm a bit concerned by all the short variable names flying around without context. Is there any way the variable names can be more descriptive?

… threading in root finding. Almost there. Of course no guarantee it compiles.
@Yurlungur
Copy link
Collaborator Author

I'm sure with a paper and some time I could decipher much of this, but as it stands I'm a bit concerned by all the short variable names flying around without context. Is there any way the variable names can be more descriptive?

👍 I blindly copied this out of the reference implementation---I will add some documentation in sphinx as well as some comments pointing to the reference implementation an the paper. Would that be sufficient to clarify?

@jhp-lanl
Copy link
Collaborator

I'm sure with a paper and some time I could decipher much of this, but as it stands I'm a bit concerned by all the short variable names flying around without context. Is there any way the variable names can be more descriptive?

👍 I blindly copied this out of the reference implementation---I will add some documentation in sphinx as well as some comments pointing to the reference implementation an the paper. Would that be sufficient to clarify?

I think that will be helpful, but I just don't feel that reading the paper should be needed for the code review. Some basic comments to explain the general structure of the code and explanations for what blocks of variables are doing (or more descriptive variable names) will probably aid the review process.

@Yurlungur
Copy link
Collaborator Author

I think that will be helpful, but I just don't feel that reading the paper should be needed for the code review. Some basic comments to explain the general structure of the code and explanations for what blocks of variables are doing (or more descriptive variable names) will probably aid the review process.

I can do that. The variable names are, for the most part, meaningfully self consistent. I think some comments explaining the convention will clarify.

@Yurlungur Yurlungur added the enhancement New feature or request label Jun 30, 2023
@AlexHls
Copy link
Collaborator

AlexHls commented Aug 17, 2023

I added a Fortran interface, but I could not test it. Compiling (more precisely linking) my test program lead to quite a few error messages. I'm not sure if is because I forgot something in the interface or due to the issue described in #284 (given that the error message mentions several EOS is suspect the latter). I would need someone else to have a look at the interface to be sure.

@AlexHls
Copy link
Collaborator

AlexHls commented Aug 17, 2023

(This part is for archival purposes, i.e. that there is a reference somewhere that we thoroughly tested the implementation against a reference implementation)

Testing

Table interpolation

  • Tested a 100x100 grid of (rho, T) values covering the table range, avoiding the edges.
  • Compared all relevant quantities that singularity-eos computes against a reference implementation (Fig 1).
  • The tests here do not consider Coulomb corrections as the cutoff is handled differently in the reference implementation (i.e. it just cuts the corrections off at a certain point, whereas we implement a butterworth filter to achieve a smooth transition)
  • A Figure is included to illustrate the effects of Coulomb corrections (Fig. 2). One can see that once the parameter space approaches the cut-off region, the difference between the implementations becomes substantial.

TableInterpolation_NoCoulombCorr
Figure 1
TableInterpolation_WithCoulombCorr
Figure 2

Root finding

The root finding is checked rather through a self-consistency check than a comparison against a reference implementation. We did compare it against the reference implementation, however, due to the different root finding algorithms and slightly different bounding mechanisms this comparison is rather difficult to interpret.

Here we test self-consistency by going 'full circle', i.e. we calculate the internal energy through the table interpolation
$$e_{int} = eos(\rho, T)$$ which is used as input for the root find: $$T_{new} = eos(\rho, e_{int})$$.
This new temperature should be identical with the input temperature; all other quantities will be computed from this quantity. As long as $T_{new}$ is correct the other quantities will be computed correctly as long as the table interpolation itself works correctly.

Fig 3. shows the difference between the original value and the value obtained from the root find without an initial guess for the temperature. It can be seen that the root find struggles in the low temperature - low density region. But once a sensible initial guess is provided (which it realistically will be in a production scenario) the root find works much better (see Fig 4).

(Both Figures also show $e_{int,new} = eos(\rho, T_{new})$ vs $e_{int}$ to illustrate that once the correct temperature is obtained, other quantities will be computed correctly.)

RootFinding_NoInitialGuess
Figure 3.

RootFinding_WithInitialGuess
Figure 4.

@@ -427,6 +443,21 @@ integer function init_sg_NobleAbel_f(matindex, eos, gm1, Cv, &
err = init_sg_NobleAbel(matindex-1, eos%ptr, gm1, Cv, bb, qq, &
c_loc(sg_mods_enabled), c_loc(sg_mods_values))
end function init_sg_NobleAbel_f

integer function init_sg_Helmholtz_f(matindex, eos, filename, rad, gas, coul, ion, ele, &
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The fortran + C++ backend looks correct. It's also not necessarily needed for the astrophysical EOS's unless desired.

@dholladay00 does this look right to you?

using Catch::Matchers::WithinRel;
using singularity::Helmholtz;
const std::string filename = "../test/helmholtz/helm_table.dat";
SCENARIO("Helmholtz equation of state - Table interpolation (tgiven)", "[HelmholtzEOS]") {
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'm not sure why either test would work---you weren't testing on device, because you had normal for loops for the test code, not portableFor loops. I've fixed it, and tests now run on device.

@Yurlungur
Copy link
Collaborator Author

From my perspective, this is ready to go. @mauneyc-LANL are you happy with the current cmake?

doc/sphinx/src/models.rst Show resolved Hide resolved
@jhp-lanl
Copy link
Collaborator

I requested changes but I'll just approve for now since I don't want to hold anything up if I'm not around

cmake/install.cmake Outdated Show resolved Hide resolved
Copy link
Collaborator

@mauneyc-LANL mauneyc-LANL left a comment

Choose a reason for hiding this comment

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

Only one issue to address, everything else looks ok.

test/CMakeLists.txt Show resolved Hide resolved
@mauneyc-LANL mauneyc-LANL dismissed their stale review August 23, 2023 21:24

Misread the code, changes not needed

@Yurlungur
Copy link
Collaborator Author

Triggered tests on re-git. Merging after they pass.

@Yurlungur Yurlungur merged commit 99c4054 into main Aug 25, 2023
4 checks passed
@Yurlungur Yurlungur deleted the jmm/helmholtz branch August 25, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants