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

MGUsup #331

Merged
merged 14 commits into from
Jan 4, 2024
Merged

MGUsup #331

merged 14 commits into from
Jan 4, 2024

Conversation

aematts
Copy link
Collaborator

@aematts aematts commented Jan 3, 2024

PR Summary

Included code and documentation for a Mie-Gruneisen EOS based on a linear Us-up relation.
It is a full EOS with consistent temperature.

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.
  • 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.

Ann Elisabet Wills - 298385 and others added 2 commits December 5, 2023 11:52
@aematts aematts self-assigned this Jan 3, 2024
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.

Great documentation @aematts ! You're also welcome to put a blurb in the other Gruniesen EOS to encourage the user to use the thermodynamically consistent one if they are able. It's up to you though whether you want to add this.

As long as things compile, this looks good to me!

Actually I want to take a slightly closer look real quick.

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

@aematts as this is marked WIP do you want review now? Or do you want us to wait? I see @jhp-lanl already reviewed. :)

@aematts
Copy link
Collaborator Author

aematts commented Jan 3, 2024

@aematts as this is marked WIP do you want review now? Or do you want us to wait? I see @jhp-lanl already reviewed. :)

I want reviews but not merge just yet. I am fixing the documentation since I am only using the pipeline for compiling it. So I need to do some commits more. I will remove the WIP when I am ready for merge.

@aematts
Copy link
Collaborator Author

aematts commented Jan 3, 2024

And both of you: THANKS for getting on it so fast.

@Yurlungur
Copy link
Collaborator

@aematts as this is marked WIP do you want review now? Or do you want us to wait? I see @jhp-lanl already reviewed. :)

I want reviews but not merge just yet. I am fixing the documentation since I am only using the pipeline for compiling it. So I need to do some commits more. I will remove the WIP when I am ready for merge.

👍 Understood. I will make sure to review this by the end of today.

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.

I think it would be good to compute a maximum density for this EOS to ensure that sound speeds aren't imaginary.

You can see we used this approach in the other Gruneisen EOS, but with a much more complicated procedure for calculating the maximum density (since it's a cubic Us-up fit).

I would recommend mirroring this approach where the user is allowed to provide a maximum density or not. If the maximum density is used, then we set it and perhaps warn the user if it's greater than the singularity in the denominator of the Hugoniot expression. If the user does not provide a maximum density, then we provide one based off of the Hugoniot fit parameter.

See this section of the other Gruneisen EOS for an example of how to do this.

Gruneisen(const Real C0, const Real s1, const Real s2, const Real s3, const Real G0,

singularity-eos/eos/eos_mgusup.hpp Show resolved Hide resolved
singularity-eos/eos/eos_mgusup.hpp Show resolved Hide resolved
@aematts
Copy link
Collaborator Author

aematts commented Jan 3, 2024

I have two more questions:

  1. where do I find the correct copyright text for updating the copyright notice?
  2. I do not know how to run on the GPUs on Darwin so can someone else make sure this run correctly on Darwin?
    (There are a few failed tests that I have not touched at all and they failed already before I started working on the files. What do I do with these?)

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Jan 3, 2024

1. where do I find the correct copyright text for updating the copyright notice?

Just copy the old one and update the date to 2024. Just worry about the files you change. We'll do an automated update later.

2. I do not know how to run on the GPUs on Darwin so can someone else make sure this run correctly on Darwin?
   (There are a few failed tests that I have not touched at all and they failed already before I started working on the files. What do I do with these?)

It's easiest to use the gitlab CI. I'll post some instructions in mattermost for how to do this.

@Yurlungur
Copy link
Collaborator

I have two more questions:

1. where do I find the correct copyright text for updating the copyright notice?

All this means is that in each file you touch, make sure that there's a blurb like the one below. And that the date includes the current calendar year as the end date... e.g., 2024.

//------------------------------------------------------------------------------
// © 2021-2024. Triad National Security, LLC. All rights reserved.  This
// program was produced under U.S. Government contract 89233218CNA000001
// for Los Alamos National Laboratory (LANL), which is operated by Triad
// National Security, LLC for the U.S.  Department of Energy/National
// Nuclear Security Administration. All rights in the program are
// reserved by Triad National Security, LLC, and the U.S. Department of
// Energy/National Nuclear Security Administration. The Government is
// granted for itself and others acting on its behalf a nonexclusive,
// paid-up, irrevocable worldwide license in this material to reproduce,
// prepare derivative works, distribute copies to the public, perform
// publicly and display publicly, and to permit others to do so.
//------------------------------------------------------------------------------
2. I do not know how to run on the GPUs on Darwin so can someone else make sure this run correctly on Darwin?

Yeah I will trigger the tests. I can also walk you through how to do it at some point. The way to do it is to push your branch to the mirror on re-git and create a pull request there. We never merge that PR but it will trigger the Darwin tests and report their status. I did it for this branch here:
https://re-git.lanl.gov/xcap/oss/singularity-eos/-/merge_requests/92

   (There are a few failed tests that I have not touched at all and they failed already before I started working on the files. What do I do with these?)

I don't see any failed tests at least reported on github? Were they in the full test suite? We'll see what it says on Darwin.

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 overall, this looks great! I have a few small nitpicks, but from my perspective this is basically good to go.

Whatever we decide to do about the compression limit, we should document. It's fine for things to fail in compression at runtime for now, but we should warn in the documentation. And we should think about a more robust solution longer term. For my money I would prefer if the code failed at initialization or not at all, as this would help deal with the garbage inputs that we are likely to receive from host codes.

doc/sphinx/src/models.rst Outdated Show resolved Hide resolved
test/test_eos_mgusup.cpp Outdated Show resolved Hide resolved
test/test_eos_mgusup.cpp Show resolved Hide resolved
singularity-eos/eos/eos_mgusup.hpp Outdated Show resolved Hide resolved
singularity-eos/eos/eos_mgusup.hpp Show resolved Hide resolved
singularity-eos/eos/eos_mgusup.hpp Show resolved Hide resolved
@Yurlungur
Copy link
Collaborator

@aematts let us know when you'd like a re-review, so we don't miss it.

@aematts aematts changed the title [WIP] MGUsup MGUsup Jan 4, 2024
@aematts
Copy link
Collaborator Author

aematts commented Jan 4, 2024

@jhp-lanl and @Yurlungur: The PR is now ready for final review.

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.

We can leave the discussion about what to do in compression for later. I am willing to approve and merge this now, pending @jhp-lanl 's approval.

Thanks for you work on this @aematts !

@aematts
Copy link
Collaborator Author

aematts commented Jan 4, 2024

Can you NOT delete my branch, please. I would like to use it again for the PowerMG.

@Yurlungur
Copy link
Collaborator

Can you NOT delete my branch, please. I would like to use it again for the PowerMG.

I changed the repository settings, the branch shouldn't be automatically deleted now.

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Jan 4, 2024

Can you NOT delete my branch, please. I would like to use it again for the PowerMG.

I changed the repository settings, the branch shouldn't be automatically deleted now.

Just to be clear here though, the branch is only deleted from the remote repository on github/re-git. The branch remains in your local checkout, so you can continue working on the branch even after it has been merged. Whenever you push the branch again, it will then exist in the remote repo as well.

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.

I created an issue about the maximum compressibility issue (see #332) so we can address it in the future. I'll approve the code as-is

@Yurlungur Yurlungur merged commit 2968efd into main Jan 4, 2024
4 checks passed
@aematts
Copy link
Collaborator Author

aematts commented Jan 5, 2024

Thank you guys! @jhp-lanl , that was exactly how I did it last time but I just did not want to do it again since I will go on with PowerMG immediately.

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.

3 participants