yt-project / yt

Main yt repository
http://yt-project.org
Other
455 stars 272 forks source link

Be careful about simple ideal/gamma-law gasisms #912

Open yt-fido opened 9 years ago

yt-fido commented 9 years ago

Originally reported by: Chris Malone (Bitbucket: ChrisMalone, GitHub: ChrisMalone)


This came up in the comments of a PR. In essence, @mzingale summarized the issue quite well:

"I'm a little concerned about the name thermal energy. We don't necessarily have thermal energy easily available [in Maestro]. We have internal energy, or gas energy, but when we are very degenerate, the internal energy is not thermal, so the name thermal energy is misleading and perhaps dangerous. Since this is a yt convention, maybe for these codes we should return an error if thermal energy is requested and perhaps we can define a more code-agnostic general name for the gas internal energy that can be used in situations where thermal energy is wrong."

Another place this crops up is in calculations of the sound speed, where \gamma might be assumed spatially constant.


yt-fido commented 9 years ago

Original comment by Chris Malone (Bitbucket: ChrisMalone, GitHub: ChrisMalone):


@jzuhone I think that would be a great contribution and a step in the right direction towards removing some of the assumptions that are tacitly made throughout the code.

yt-fido commented 9 years ago

Original comment by John ZuHone (Bitbucket: jzuhone, GitHub: jzuhone):


So I'm happy, with input from @mzingale, to take this one on too--it seems like what needs to happen is the following:

  1. Opt for an "internal_energy" field instead of a "thermal_energy" field, with perhaps an alias for backwards-compatibility for some frontends.
  2. Define a "gamma" field that is either spatially dependent or just the scalar gamma multiplied by np.ones, depending on the simulation and the frontend.

As far as a more general computation of the sound speed, that would probably take some more thought, but this also might need to be frontend-dependent.

yt-fido commented 9 years ago

Original comment by Matt Turk (Bitbucket: MatthewTurk, GitHub: MatthewTurk):


I think we should provide reasonable defaults and encourage overriding. But, using gamma as a scalar is not correct in general. I think for ARTIO we use a real gamma field.

yt-fido commented 9 years ago

Original comment by Michael Zingale (Bitbucket: mzingale, GitHub: Unknown):


also, that definition of sound speed would not be correct for special relativistic hydro codes.

yt-fido commented 9 years ago

Original comment by Michael Zingale (Bitbucket: mzingale, GitHub: Unknown):


and it looks like in field_detector.py we set it to 5./3.

Otherwise, ack shows only the frontends have the variable gamma in them.

yt-fido commented 9 years ago

Original comment by Michael Zingale (Bitbucket: mzingale, GitHub: Unknown):


In fluid_fields.py:_sound_speed() it looks like it does:

    def _sound_speed(field, data):
        tr = data.ds.gamma * data[ftype, "pressure"] / data[ftype, "density"]
        return np.sqrt(tr)
    registry.add_field((ftype, "sound_speed"),
             function=_sound_speed,
             units="cm/s")
yt-fido commented 9 years ago

Original comment by Matt Turk (Bitbucket: MatthewTurk, GitHub: MatthewTurk):


If it assumes that in any derived fields, we should immediately correct it. That's not correct even for Enzo if H2 is included.

yt-fido commented 9 years ago

Original comment by Michael Zingale (Bitbucket: mzingale, GitHub: Unknown):


Related to this is that yt assumes a constant gamma in places.