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

Unify behavior of DensityEnergyFromPressureTemperature #199

Open
Yurlungur opened this issue Nov 30, 2022 · 5 comments
Open

Unify behavior of DensityEnergyFromPressureTemperature #199

Yurlungur opened this issue Nov 30, 2022 · 5 comments
Labels
clean-up Changes that don't affect code execution but make the code cleaner

Comments

@Yurlungur
Copy link
Collaborator

This function is unused and just for convenience. It also may hinder future development for multiphase and we should remove it.

@Yurlungur Yurlungur added the clean-up Changes that don't affect code execution but make the code cleaner label Nov 30, 2022
@jhp-lanl
Copy link
Collaborator

@Yurlungur from the mattermost chat, we have also brought up that this function is mainly useful for initialization from a pressure-temperature state where the desired density is the solid density.

I still support deleting the EOS-specific versions and maybe instead using the same strategy we use for the PTE solvers where we try to find ambient density based off of the density at the pressure minimum. This could be a generic behavior that could live in the CRTP base class. We may not get as nice of values for analytic EOS, but I think it would still be acceptable for initialization.

@Yurlungur
Copy link
Collaborator Author

Ah good point. I like the CRTP idea.

@jhp-lanl jhp-lanl mentioned this issue Dec 13, 2022
2 tasks
@jhp-lanl jhp-lanl changed the title Delete DensityEnergyFromPressureTemperature Unify behavior of DensityEnergyFromPressureTemperature Mar 1, 2023
@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Mar 1, 2023

In #233 we discussed the fact that different EOS have different behaviors for this function. For example, the Davis EOS use the ambient density as an initial guess in the root find while JWL allows specification of an initial guess through the density and spiner allows the initial guess through the lambda parameter. For analytic EOS, this behavior can probably be unified in the base class.

It may also be worthwhile to provide this functionality for EOSPAC as well since the EOSPAC P-T lookup can sometimes produce strange values for some EOS and may benefit from an external root find in rho-T space.

@Yurlungur
Copy link
Collaborator Author

I return to the thought that maybe we should delete this function---perhaps the preferred path here is to use the PTE machinery... or to provide external root-finding capability specifically for this that has a clearer API.

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Mar 2, 2023

perhaps the preferred path here is to use the PTE machinery... or to provide external root-finding capability specifically for this that has a clearer API.

The PTE machinery is not built for this; the PTE machinery recieves a density and energy and returns a pressure and temperature.

I will argue that it is imperative that we provide some functionality that produces the energy and density at a given pressure and temperature. I'd like to hear your ideas on why it shouldn't live in the EOS itself though. From my perspective it seems natural to have this in the EOS, but we need to decide on the appropriate procedure for providing a guess or ensuring predictable behavior though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean-up Changes that don't affect code execution but make the code cleaner
Projects
None yet
Development

No branches or pull requests

2 participants