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

Apg/sap integration #278

Merged
merged 68 commits into from
Nov 22, 2023
Merged

Apg/sap integration #278

merged 68 commits into from
Nov 22, 2023

Conversation

annapiegra
Copy link
Collaborator

@annapiegra annapiegra commented Jul 25, 2023

PR Summary

Creates a new f90-interface needed for the Pagosa integration.

-Adds eos_setOptions
-Decouples eospac preinversion and insert data
-Uses a single step instead of two to get sesame P from D+Ut

TODO: docs, tests, fix properly singularity_eos.f90 spiner deps...

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

@annapiegra I fixed a couple errors that were breaking the build. I also added the MinInternalEnergyFromDensity() function to the scratch buffer size querrying mechanism. Currently I'm assuming EOSPAC either won't write to the second derivative array (dFy in EOSPAC parlance) or that because we don't care about that value, nothing nasty will happen when it does. Feel free to add another scratch buffer if you think that's going to be an issue.

@annapiegra
Copy link
Collaborator Author

annapiegra commented Jul 26, 2023 via email

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 this seems reasonable to me. I'd like Richard's eyes on the EOSPAC stuff and I would also like to make sure all the PR checklist boxes are checked. But barring that, I approve. A few nitpicks below.

singularity-eos/eos/eos_gruneisen.hpp Show resolved Hide resolved
eospac-wrapper/eospac_wrapper.cpp Outdated Show resolved Hide resolved
eospac-wrapper/eospac_wrapper.cpp Outdated Show resolved Hide resolved
@jhp-lanl jhp-lanl mentioned this pull request Aug 3, 2023
5 tasks
@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Nov 16, 2023

@cmauney @Yurlungur @dholladay00 @rbberger can I interpret your silence for approval?

EDIT - @Yurlungur already approved

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

@rbberger rbberger 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 good to me. Just some minor things I found.

@jhp-lanl
Copy link
Collaborator

Tests pass on gitlab as well as github. Merging.

@jhp-lanl jhp-lanl merged commit b08627a into main Nov 22, 2023
4 checks passed
@jhp-lanl jhp-lanl deleted the apg/sap_integration branch November 22, 2023 01:37
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