Add MHD Support to Projection and Rotated Projection Outputs #366
Conversation
bcaddy
force-pushed
the
dev-mhdProjections
branch
from
2cabdc3 to
ddd046e
Compare
The gas energy was being loaded then checked for correctness against a variable that didn't exist. The gas energy was also being updated when it shouldn't be. Fixed those bugs and added MHD support to the temperature calculations. Also, updated the hydro_utilities::Calc_Temp_DE function to use the total, not specific, gas energy and added documentation for clarity
bcaddy
force-pushed
the
dev-mhdProjections
branch
from
8c7d46e to
80cd55e
Compare
src/utils/hydro_utilities.h
Outdated
| /*! | ||
| * \brief Compute the temperature when DE is turned on | ||
| * | ||
| * \param total_gas_energy The total gas energy in the cell. This is the value stored in the grid |
There was a problem hiding this comment.
Might be clearer to specifically say the name of the grid_enum index where the value is stored
There was a problem hiding this comment.
Also, I'm not sure that total_gas_energy is the clearest variable name. In the style guide, that's a combination of two different variable names we decided on for different forms of energy (gas_energy and total_energy). I'm a little unclear on which one would be correct to use here (we should probably clarify that in the naming guide)
There was a problem hiding this comment.
I don't think we want energy_tot, since that would imply the total energy, i.e. thermal + kinetic + magnetic etc. etc. My reading of the style guide is that it should be total_gas_energy.
There was a problem hiding this comment.
My impression is that the total is not actually necessary, since we usually mean the total when we say any of these (energy, kinetic energy, etc.) and really what we should be doing is clarifying when we are using the specific version (i.e. the old ge should be specific_gas_energy).
There was a problem hiding this comment.
I don't think we want energy_tot, since that would imply the total energy, i.e. thermal + kinetic + magnetic etc. etc. My reading of the style guide is that it should be total_gas_energy.
It is total_gas_energy I'm confused
My impression is that the total is not actually necessary...
Sure, but I'd rather be extra clear since the ambiguity is what led to this multi-hour debacle.
There was a problem hiding this comment.
In fact, I don't see energy_total anywhere in the code, and would advocate for this approach. I think gas_energy is consistent with the way this is labeled elsewhere (i.e. in reconstruction.h).
There was a problem hiding this comment.
Sorry, I think I created a bit of confusion because I was still editing my comments before I realized that you guys were replying. However, I agree that the total_ isn't that clear. I think the clarification in the doc string and following what's done elsewhere in the code by calling it gas_energy should alleviate any confusion.
There was a problem hiding this comment.
I've changed the variable name to gas_energy.
gas_energy is the total gas energy and specific_gas_energy is the specific version
Now you only use magnetic variables when MHD is enabled
There was a problem hiding this comment.
I think it would be more consistent if n and T were treated the same way between the two versions (projections versus rotated projections), since in the first function d and n and T are all redefined as Real const within the loop and in the second only T is, which makes them superficially appear to be doing different things. I also think it would be clearer if the non-MHD case didn't define the empty B-fields (as we discussed offline), so that it is clear the Calc_Temp function can be called without them. Otherwise, I think this is ready to go.
|
I believe those things are done now. |
|
Everything is passing except a single clang-tidy warning in unrelated code. I don't know why that warning didn't pop up in a previous PR but it is fixed in PR #364. IMO, either that PR should be merged in first or this one is OK to merge. |
Summary
Per the discussion in PR #365 I've added MHD support to the projection and rotated projection outputs along with a minor refactor to those functions to accomplish this and utilize more of the utility functions we've written.
I also added a utility function for computing the temperature from the conserved variables (
hydro_utilities::Calc_Temp_Conserved). Internally it simply callshydro_utilities::Calc_Pressure_Conservedandhydro_utilities::Calc_Tempto compute the temperature.Duel Energy + Dust Fixes
Dust_Kernelhad few DE related bugs in it that I fixed with help from @helenarichie. While doing that I also slightly redefinedhydro_utilities::Calc_Temp_DEto take the total gas energy instead of the specific gas energy and density.