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

Rewrite dedT in gamma_law EOS #1298

Closed
wants to merge 13 commits into from
Closed

Conversation

psharda
Copy link
Collaborator

@psharda psharda commented Jul 24, 2023

Hi @zingale , is it okay if we rewrite state.dedT in this way? It is identical to the original expression, but doesn't directly use other state quantities.

If we can make this change, it would make it easy to extract dedT from Microphysics for Quokka without needing to supply a temperature.

@BenWibking
Copy link
Collaborator

For a constant-gamma EOS, this works. We still need to compute $de/dT$ for more general EOSes, though, so I'm not sure how useful this is.

@psharda
Copy link
Collaborator Author

psharda commented Jul 24, 2023

For a constant-gamma EOS, this works. We still need to compute de/dT for more general EOSes, though, so I'm not sure how useful this is.

If we don't do this, then the call to get dedT from Microphysics in ComputeEintTempDerivative in EOS.hpp will return a NAN, because we don't give it a temperature as an input.

@BenWibking
Copy link
Collaborator

For a constant-gamma EOS, this works. We still need to compute de/dT for more general EOSes, though, so I'm not sure how useful this is.

If we don't do this, then the call to get dedT from Microphysics in ComputeEintTempDerivative in EOS.hpp will return a NAN, because we don't give it a temperature as an input.

We could give it the temperature as an input, no? Or is this not possible to give it the temperature for some reason?

@psharda
Copy link
Collaborator Author

psharda commented Jul 24, 2023

For a constant-gamma EOS, this works. We still need to compute de/dT for more general EOSes, though, so I'm not sure how useful this is.

If we don't do this, then the call to get dedT from Microphysics in ComputeEintTempDerivative in EOS.hpp will return a NAN, because we don't give it a temperature as an input.

We could give it the temperature as an input, no? Or is this not possible to give it the temperature for some reason?

Well, if we wanted to keep the current structure of this function as it is, we don't want to give temperature as an input. My aim is to not have to change the function arguments.

@BenWibking
Copy link
Collaborator

I don't know what you're referring to. For the function in EOS.hpp, the temperature is already a function argument: https://github.com/quokka-astro/quokka/blob/1b572e70c1ab71d40e8c7f8650d9c90f4fcbe0df/src/EOS.hpp#L133

@psharda
Copy link
Collaborator Author

psharda commented Jul 24, 2023

const amrex::Real /*Tgas*/ - does it mean Tgas is accessible to the funtion even though there is a /* */ around it? Also, can't another file call this function without giving it a temperature?

@BenWibking
Copy link
Collaborator

const amrex::Real /*Tgas*/ - does it mean Tgas is accessible to the funtion even though there is a /* */ around it? Also, can't another file call this function without giving it a temperature?

You just have to uncomment it. It's currently commented with /* */ because it was previously unused. A temperature should be passed to this function anywhere it's currently used -- I don't think that's an issue.

@psharda
Copy link
Collaborator Author

psharda commented Jul 24, 2023

You just have to uncomment it. It's currently commented with /* */ because it was previously unused. A temperature should be passed to this function anywhere it's currently used -- I don't think that's an issue.

Yes, this is what I wanted to not change. But it's a small change anyway so okay, I will do that and then we don't need this PR.

@psharda psharda closed this Jul 24, 2023
@psharda psharda deleted the new-amu branch September 23, 2023 13:19
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