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

Create minimum energy modifier #375

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

Create minimum energy modifier #375

wants to merge 29 commits into from

Conversation

jhp-lanl
Copy link
Collaborator

@jhp-lanl jhp-lanl commented May 11, 2024

PR Summary

Creates a minimum energy modifier and unifies EOS behind a default behavior of returning the zero K isotherm as the minimum possible internal energy.

Some notes:

  1. I duplicated @rbberger 's approach with the shifted EOS using scratch memory. Similar to populating a shift, I populate a minimum energy and then max that with the input energy to then pass to whatever function needs the internal energy.
  2. I am assuming that all energies are valid when they are outputs rather than inputs. I feel this is reasonable since the EOS should be self-consistent enough to not return energies off the EOS surface. If there is an issue, it might point to badly bounded inputs (i.e. negative temperatures or densities).
  3. Initially this is very rough and is guaranteed to not compile. I'll need to make tests and make sure that the Gruneisen EOS can properly call the base class version within its own version and that all the other classes can properly see both the vector and scalar overloads from the base class.

Fixes #318

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

@jhp-lanl jhp-lanl marked this pull request as draft May 11, 2024 02:35
@jhp-lanl jhp-lanl changed the title WIP: Create minimum energy modifier Create minimum energy modifier May 11, 2024
@jhp-lanl
Copy link
Collaborator Author

Wow it compiled first time!? The modifier isn't compiled yet, but the default minimum energy stuff should be compiling, which might mean what I wrote works?

I'd still like to make an explicit unit test something along the lines of asserting that the energy is the zero temperature energy.

@jhp-lanl jhp-lanl requested review from Yurlungur and dholladay00 May 11, 2024 02:57
@dholladay00
Copy link
Collaborator

Would this play nicely with the options that @annapiegra added for eospac @jhp-lanl ? It sounds similar though I don't recall the details.

I assume it matters which order an energy shift is applied so that will need to be taken into account when constructing the modifier list.

@jhp-lanl
Copy link
Collaborator Author

Would this play nicely with the options that @annapiegra added for eospac @jhp-lanl ? It sounds similar though I don't recall the details.

I assume it matters which order an energy shift is applied so that will need to be taken into account when constructing the modifier list.

This is the exact function that @annapiegra added, but when it was added it was basically disabled (would throw an error) for any EOS other than EOSPAC. This basically changes that so that every EOS will have some notion of minimum energy by default.

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.

Very clean how this came out. I wonder if we should make this more "monolithic" and just provide bounds on all independent variables.

@jhp-lanl
Copy link
Collaborator Author

Very clean how this came out. I wonder if we should make this more "monolithic" and just provide bounds on all independent variables.

I assume that would mean expanding this to enforce a minimum density and temperature as well? That seems pretty straight-foward and might be pretty easy to implement.

I don't know if I'd want to do anything with upper bounds though. I feel that an extrapolation approach might be a superior way to handle upper-bounds.

Speaking of extrapolation, I suppose that if a table has a non-zero lower temperature bound we might want to apply an extrapolation layer first that extends the minimum energy/temperature to 0 K first and then this would lie on top of it.

@Yurlungur
Copy link
Collaborator

Very clean how this came out. I wonder if we should make this more "monolithic" and just provide bounds on all independent variables.

I assume that would mean expanding this to enforce a minimum density and temperature as well? That seems pretty straight-foward and might be pretty easy to implement.

I think it might be nice to impose energy and temperature floors together, yeah. 👍

I don't know if I'd want to do anything with upper bounds though. I feel that an extrapolation approach might be a superior way to handle upper-bounds.

That's fair... but is that always going to be valid going forward? What about an EOS that is for, e.g., a single phase as @aematts is planning to use?

Speaking of extrapolation, I suppose that if a table has a non-zero lower temperature bound we might want to apply an extrapolation layer first that extends the minimum energy/temperature to 0 K first and then this would lie on top of it.

True! I actually think what you've implemented here is a nice template for that kind of thing.

@jhp-lanl
Copy link
Collaborator Author

That's fair... but is that always going to be valid going forward? What about an EOS that is for, e.g., a single phase as @aematts is planning to use?

That's a good point, but I think there's an important difference: I'm constraining the input to be on the EOS surface whereas I think @aematts is going to want to constrain the output such that it's clear when a phase EOS isn't valid.

From my limited understanding of kinetic phase transitions, you might want to set a domain of validity for a phase EOS and not constrain yourself to that domain, but instead maybe provide a flag to indicate that you're not in the range of validity anymore. For example, you could return an infinite Gibbs free energy (or equivalent) when you're outside the domain of validity (which is basically a constant extrapolation). That way the rate at which that phase is created can be set to identically zero.

Either way, I anticipate that we'll have to create additional modifiers to handle these use scenarios.

@Yurlungur
Copy link
Collaborator

That's fair... but is that always going to be valid going forward? What about an EOS that is for, e.g., a single phase as @aematts is planning to use?

That's a good point, but I think there's an important difference: I'm constraining the input to be on the EOS surface whereas I think @aematts is going to want to constrain the output such that it's clear when a phase EOS isn't valid.

From my limited understanding of kinetic phase transitions, you might want to set a domain of validity for a phase EOS and not constrain yourself to that domain, but instead maybe provide a flag to indicate that you're not in the range of validity anymore. For example, you could return an infinite Gibbs free energy (or equivalent) when you're outside the domain of validity (which is basically a constant extrapolation). That way the rate at which that phase is created can be set to identically zero.

Either way, I anticipate that we'll have to create additional modifiers to handle these use scenarios.

Yeah fair enough. I may be over-engineering here.

@jhp-lanl jhp-lanl marked this pull request as ready for review February 20, 2025 17:37
@jhp-lanl
Copy link
Collaborator Author

@Yurlungur I think this is ready for final review (sorry for the long delay)

@jhp-lanl jhp-lanl requested a review from Yurlungur February 20, 2025 17:38
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.

Non-blocking request. But overall looks good!

@jhp-lanl
Copy link
Collaborator Author

Oh and I still need to test this code. I'll try to do that here shortly.

@jhp-lanl
Copy link
Collaborator Author

@jonahm-LANL It looks like SpinerEOSDependsRhoSie doesn't have the same cold curve infrastructure that SpinerEOSDependsRhoT does so I left that function disabled. It's probably faster for you to add the required functionality than it is for me.

This is ready for final review (really just the test was added, but there are a few more changes)

@jhp-lanl
Copy link
Collaborator Author

CI is failing on LLNL systems. DO NOT MERGE

@Yurlungur
Copy link
Collaborator

@jonahm-LANL It looks like SpinerEOSDependsRhoSie doesn't have the same cold curve infrastructure that SpinerEOSDependsRhoT does so I left that function disabled. It's probably faster for you to add the required functionality than it is for me.

Sounds good. I'll add it in a subsequent MR.

This is ready for final review (really just the test was added, but there are a few more changes)

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.

Looks good. One more suggestion caught. I think I fixed the problems on re-git in #473. But we should double check that before merging this.

Comment on lines +318 to +337
Notably though, tabular SESAME tables can result in unexpected behavior. The
``EOSPAC`` and ``SpinerEOSDependsRhoT`` models both rely on the result when
EOSPAC querries the cold energy table (306). This energy *can* differ from the
zero K isotherm for two reasons:

#. The zero K isotherm includes the zero point energy. In other words, it is the
energy of the lowest energy level whereas the cold curve is the energy of the
potential well itself. This effect may be insignificant.

#. In the absence of a 306 table for the material, EOSPAC will instead report
the lowest isotherm's energy in place of a cold curve. If a given table does
not extend to zero K, then an extrapolaiton is *not* performed. This results
in an energy floor from the lowest temperature on the table, **not** from
an extrapolated energy at zero K.

.. note::

This modifier is only compatible with EOS where the
``MinInternalEnergyFromDensity`` function is properly defined. Where it isn't
defined, an error will occur at runtime.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 for careful documentation here

Comment on lines +345 to +351
inline constexpr bool IsModified() const { return true; }

inline constexpr T UnmodifyOnce() { return t_; }

inline constexpr decltype(auto) GetUnmodifiedObject() {
return t_.GetUnmodifiedObject();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
inline constexpr bool IsModified() const { return true; }
inline constexpr T UnmodifyOnce() { return t_; }
inline constexpr decltype(auto) GetUnmodifiedObject() {
return t_.GetUnmodifiedObject();
}
PORTABLE_FORCEINLINE_FUNCTION Real MaximumDensity() const {
return t_.MaximumDensity();
}
PORTABLE_FORCEINLINE_FUNCTION Real MinimumPressure() const {
return t_.MinimumPressure();
}
PORTABLE_FORCEINLINE_FUNCTION Real MaximumPressureAtTemperature(const Real temp) const {
return t_.MaximumPressureAtTemperature(temp);
}
template <typename Indexer_t = Real *>
PORTABLE_INLINE_FUNCTION Real MeanAtomicMassFromDensityTemperature(
const Real rho, const Real temperature,
Indexer_t &&lambda = static_cast<Real *>(nullptr)) const {
return t_.MeanAtomicMassFromDensityTemperature(rho, temperature, lambda);
}
template <typename Indexer_t = Real *>
PORTABLE_INLINE_FUNCTION Real MeanAtomicNumberFromDensityTemperature(
const Real rho, const Real temperature,
Indexer_t &&lambda = static_cast<Real *>(nullptr)) const {
return t_.MeanAtomicNumberFromDensityTemperature(rho, temperature, lambda);
}
SG_ADD_MODIFIER_METHODS(T, t_);
SG_ADD_MODIFIER_MEAN_METHODS(t_)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah my bad, I forgot how old this had become. I'll add those

Comment on lines +86 to +104
// Helper function to copy a collection of EOS to device memory
template <typename EOSArrT, typename EOS_T>
EOS_T *copy_eos_arr_to_device(const int num_eos, EOSArrT eos_arr) {
// Assumes that GetOnDevice() has already been called for each EOS in eos_arr
const size_t EOS_bytes = num_eos * sizeof(EOS_T);
EOS_T *v_EOS = (EOS_T *)PORTABLE_MALLOC(EOS_bytes);
const size_t bytes = num_eos * sizeof(EOS_T);
portableCopyToDevice(v_EOS, eos_arr.data(), bytes);
return v_EOS;
}

// Helper function to call Finalize() on each eos in an array (host side)
template <typename EOSArrT>
void finalize_eos_arr(EOSArrT eos_arr) {
// Call Finalize on each EOS on the host
for (auto eos : eos_arr) {
eos.Finalize();
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

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.

Provide default functionality for MinInternalEnergyFromDensity()
3 participants