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

Add mean atomic mass and number #439

Merged
merged 19 commits into from
Dec 6, 2024
Merged

Add mean atomic mass and number #439

merged 19 commits into from
Dec 6, 2024

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Nov 30, 2024

PR Summary

This MR adds mean atomic mass and number to the singularity-eos API, as required for 3T physics. To be clear these are not the ionization state. Rather they are fundamental properties of the atomic nuclei that make up a material. They are not integers, as they are the average over the molecular configuration. This also means they may change under chemical or nuclear burning, either by changing the nuclei themselves or by changing the chemical composition such that the average nucleus changes.

This is required for 3T because ionization models that compute an ionization state, and pieces of approximate models such as z splitting require them. However, the ionization state is a separate quantity that will be treated elsewhere, possibly in a later MR.

Tabulated EOS's will likely retrieve mean atomic mass and number from the underlying table. Sesame tables, for example, report it in table metadata. For most EOS's these are static quantities, but for some EOS's, like the stellar collapse tables which assume nuclear statistical equilibrium, they depend on density and temperature.

To support these constraints, I define four new API functions:

Real MeanAtomicMass() const;
Real MeanAtomicNumber() const;
Real MeanAtomicMassFromDensityTemperature(const Real rho, const Real T, LambdaIndexer lambda) const;
Real MeanAtomicNumberFromDensityTemperature(const Real rho, const Real T, LambdaIndexer lambda) const;

(insert appropriate portability markings etc). The latter two have default implementations in the base class that call the former two. But a given EOS may overwrite. We may consider adding vector implementations for the latter two down the line. For now, I decided not to. But we may wish add them when we thread 3T through for the fortran codes, depending on which EOS's those codes want to use and whether or not we want to expose density/temperature dependence there.

For the analytic EOS's, there's no intrinsic value for these quantities to take. It's another free parameter, like the Gruneisen coefficient. But it is physically and meaningfully tied to the underlying material model---it's a property of the constituent atoms. For this reason, I add mean atomic mass and number to the analytic EOS's as member fields and set them in the constructor.

We don't want to require users to set the mean atomic mass and number, though, unless they need it. So we should make it an optional parameter. This could be difficult to disambiguate from other optional parameters such as reference entropy in multiple constructor overloads. So to avoid this ambiguity, and to minimize code, I define a struct

struct MeanAtomicProperties

which owns mean atomic mass and number. This struct is now passed in as the final optional argument of all analytic EOS constructors. (Though I don't think that should be a requirement.) I also take advantage of the existence of this struct as a member field to define default versions of MeanAtomicMass and MeanAtomicNumber which can be added to a class via a macro.

I'd like feedback on this design (thanks @jhp-lanl for already providing quite a bit of it!), as I'm not 100% happy with it. That said, it adds the needed capability with a fairly minimal changeset (that albeit touches almost every file).

The roadmap for 3T is here: #440 . Feel free to give feedback there too.

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.
  • Ensure that any when='@main' dependencies are updated to the release version in the package.py

@Yurlungur Yurlungur added the enhancement New feature or request label Nov 30, 2024
@Yurlungur Yurlungur self-assigned this Nov 30, 2024
@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Dec 2, 2024

Since this is built off of your other MR, I haven't reviewed everything, but I'm immediately struck by the need to modify all of the constructors to take a MeanAtomicProperties struct. To me, this might be better accomplished using a modifier rather than modifying all the EOS manually.

I also think the other issue is that it's not obvious to me right now how an ionization model (or different ionization models) would fit into this. I immediately think of a policy-based design where the ionization model is injected into the EOS, which also makes me think that a modifier that takes an ionization policy (e.g. constant ionization state) is more extensible.

I think we maybe also talk in more detail about your roadmap for 3T physics so I can understand more about the grand vision here.

@Yurlungur
Copy link
Collaborator Author

Since this is built off of your other MR, I haven't reviewed everything,

Yeah sorry about that... This one is definitely still WIP.

I'm immediately struck by the need to modify all of the constructors to take a MeanAtomicProperties struct. To me, this might be better accomplished using a modifier rather than modifying all the EOS manually.

I considered adding a setter function instead. But it didn't feel appropriate. The constructors only need to be modified for analytic EOS's. Think of this like adding a reference entropy. It's something the user should be able to set and the EOS should be able to report. That's why I didn't make it a modifier.

I also think the other issue is that it's not obvious to me right now how an ionization model (or different ionization models) would fit into this. I immediately think of a policy-based design where the ionization model is injected into the EOS, which also makes me think that a modifier that takes an ionization policy (e.g. constant ionization state) is more extensible.

This piece is not about the ionization model---it's sort of a prerequisite. We need to know mean atomic mass and number for things like building a Zplit modifier that partially ionizes an EOS.

I think we maybe also talk in more detail about your roadmap for 3T physics so I can understand more about the grand vision here.

Sure let's talk about it at some point. I created an issue #440 describing a roadmap. But we should also discuss.

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Dec 3, 2024

I considered adding a setter function instead. But it didn't feel appropriate. The constructors only need to be modified for analytic EOS's. Think of this like adding a reference entropy. It's something the user should be able to set and the EOS should be able to report. That's why I didn't make it a modifier.

I'm not sure I agree since both abar and zbar could both be dynamic quantities (the former in the case of fusion for example). A reference entropy is something that I can't imagine being dynamic.

That said, you could consider a modifier type of approach for entropy, but another difference is that entropy is tightly connected to the EOS itself; an incomplete EOS doesn't really have a concept of entropy whereas a complete EOS does. I see 3T looking more like an energy shift which makes me think a modifier is more appropriate.

This piece is not about the ionization model---it's sort of a prerequisite. We need to know mean atomic mass and number for things like building a Zplit modifier that partially ionizes an EOS.

I get that, but I'm trying to think about the eventual dynamic case where we want to change their values (e.g. via an ionization model). I'm struggling to see what that would look like.

With a modifier approach, I feel like it would be easy to inject the ionization model into the modifier class via the constructor. That could take a state and return an ionization that could be used in calculating the pressure (I'm not sure if this would have to be iterative at this point...).

There's also the possibility that the host code could be dynamically changing at least abar on its own somehow and we'd need to support allowing that as an input (probably a lambda parameter?).

Anyway, it's not clear to me how this sort of interaction would take place.

@Yurlungur
Copy link
Collaborator Author

I'm not sure I agree since both abar and zbar could both be dynamic quantities (the former in the case of fusion for example). A reference entropy is something that I can't imagine being dynamic.

Agreed. I added support for this. The base class implements MeanAtomicMassFromDensityTemperature and MeanAtomicNumberFromDensityTemperature that by default return the constructed values but an individual EOS may do something more sophisticated. For example, Stellar Collapse assumes nuclear statistical equilibrium and tabulates mean atomic mass and number. The Helmholtz EOS takes them as an input.

For both of these models, calling the temperature independent function raises an error. You must call the dependent one.

That said, you could consider a modifier type of approach for entropy, but another difference is that entropy is tightly connected to the EOS itself; an incomplete EOS doesn't really have a concept of entropy whereas a complete EOS does. I see 3T looking more like an energy shift which makes me think a modifier is more appropriate.

Yeah for truly 3T stuff, I agree. But IMO the mean atomic mass and number are (for non-reactive EOS's) pretty intrinsically tied to the EOS since they are fundamental properties of the chemical structure of the material. For reactive EOS's, that's less clear. But that's why I try to support the density/temperature-dependent model too.

I get that, but I'm trying to think about the eventual dynamic case where we want to change their values (e.g. via an ionization model). I'm struggling to see what that would look like.

Ah, I think I see the confusion. Abar and Zbar are overloaded terms. The Abar and Zbar here don't change under ionization. These are the fundamental properties of the material. This is not the Z in the Zsplit. That is the ionization state. This is the average number of protons in the nucleus. We sometimes need these properties for computing the ionization state. For example, the Thomas-Fermi model takes the mean atomic mass and number as inputs.

With a modifier approach, I feel like it would be easy to inject the ionization model into the modifier class via the constructor. That could take a state and return an ionization that could be used in calculating the pressure (I'm not sure if this would have to be iterative at this point...).

👍 Yeah I agree that for ionization state we want to handle this with a modifier. Likely, as you say, by passing the ionization state in through the lambda.

@Yurlungur
Copy link
Collaborator Author

That said I could be convinced to remove the temperature-independent values for the mean atomic properties, since in full generality the temperature dependent ones are the right ones. But it seems like a useful convenience feature. IMO it's best to just warn users to use the dependent versions when they expect chemical properties to change via, e.g., burning.

@Yurlungur Yurlungur changed the title [WIP] Add mean atomic mass and number Add mean atomic mass and number Dec 3, 2024
@Yurlungur
Copy link
Collaborator Author

This isn't completely ready but I'm removing the WIP marking. I will also update the above description with some comments about the overall design and intent.

@Yurlungur Yurlungur requested a review from jdolence December 3, 2024 02:29
@Yurlungur
Copy link
Collaborator Author

Updated MR summary

This MR adds mean atomic mass and number to the singularity-eos API, as required for 3T physics. To be clear these are not the ionization state. Rather they are fundamental properties of the atomic nuclei that make up a material. They are not integers, as they are the average over the molecular configuration. This also means they may change under chemical or nuclear burning, either by changing the nuclei themselves or by changing the chemical composition such that the average nucleus changes.

This is required for 3T because ionization models that compute an ionization state, and pieces of approximate models such as z splitting require them. However, the ionization state is a separate quantity that will be treated elsewhere, possibly in a later MR.

Tabulated EOS's will likely retrieve mean atomic mass and number from the underlying table. Sesame tables, for example, report it in table metadata. For most EOS's these are static quantities, but for some EOS's, like the stellar collapse tables which assume nuclear statistical equilibrium, they depend on density and temperature.

To support these constraints, I define four new API functions:

Real MeanAtomicMass() const;
Real MeanAtomicNumber() const;
Real MeanAtomicMassFromDensityTemperature(const Real rho, const Real T, LambdaIndexer lambda) const;
Real MeanAtomicNumberFromDensityTemperature(const Real rho, const Real T, LambdaIndexer lambda) const;

(insert appropriate portability markings etc). The latter two have default implementations in the base class that call the former two. But a given EOS may overwrite. We may consider adding vector implementations for the latter two down the line. For now, I decided not to. But we may wish add them when we thread 3T through for the fortran codes, depending on which EOS's those codes want to use and whether or not we want to expose density/temperature dependence there.

For the analytic EOS's, there's no intrinsic value for these quantities to take. It's another free parameter, like the Gruneisen coefficient. But it is physically and meaningfully tied to the underlying material model---it's a property of the constituent atoms. For this reason, I add mean atomic mass and number to the analytic EOS's as member fields and set them in the constructor.

We don't want to require users to set the mean atomic mass and number, though, unless they need it. So we should make it an optional parameter. This could be difficult to disambiguate from other optional parameters such as reference entropy in multiple constructor overloads. So to avoid this ambiguity, and to minimize code, I define a struct

struct MeanAtomicProperties

which owns mean atomic mass and number. This struct is now passed in as the final optional argument of all analytic EOS constructors. (Though I don't think that should be a requirement.) I also take advantage of the existence of this struct as a member field to define default versions of MeanAtomicMass and MeanAtomicNumber which can be added to a class via a macro.

I'd like feedback on this design (thanks @jhp-lanl for already providing quite a bit of it!), as I'm not 100% happy with it. That said, it adds the needed capability with a fairly minimal changeset (that albeit touches almost every file).

The roadmap for 3T is here: #440 . Feel free to give feedback there too.

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Dec 3, 2024

Thanks for clearing up my confusion and adding more information! I'll think about this more tomorrow.

@Yurlungur
Copy link
Collaborator Author

This now ready for review.

@Yurlungur Yurlungur mentioned this pull request Dec 5, 2024
4 tasks
@Yurlungur
Copy link
Collaborator Author

All tests now pass on re-git.

Copy link
Collaborator

@pdmullen pdmullen left a comment

Choose a reason for hiding this comment

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

LGTM! Non-blocking comments...

doc/sphinx/src/models.rst Show resolved Hide resolved
singularity-eos/eos/eos_ideal.hpp Show resolved Hide resolved
@Yurlungur
Copy link
Collaborator Author

@jhp-lanl do you think you'll have a chance to look at this today?

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Dec 5, 2024

@jhp-lanl do you think you'll have a chance to look at this today?

I'll look now

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 don't have any objections. Looks good!

// This macro adds these methods to a derived class. Due to scope,
// these can't be implemented in the base class, unless we make
// _AZbar public. Not all EOS's may want these default functions
// TODO(JMM): Should we go the alternate route and make _AZbar public?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like all the copy paste everywhere, but admittedly the constructor changes are probably a bigger issue so I'm fine keeping it this way

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 agree... Let's discuss offline when we get the chance. Not too late to change it if we see a better path.

@Yurlungur
Copy link
Collaborator Author

Thanks for the reviews, @jhp-lanl @pdmullen ! Merging now. But as discussed, I recognize this is a compromise solution. If there's a better path, not too late to change it in a subsequent MR.

@Yurlungur Yurlungur merged commit b483579 into main Dec 6, 2024
9 checks passed
@Yurlungur Yurlungur deleted the jmm/mean-atomic-mass branch December 6, 2024 03:58
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.

4 participants