Cluster improvements and donut model #82
Conversation
| using parthenon::TE; | ||
| using parthenon::TopologicalElement; | ||
|
|
||
| struct ProlongateCellMinModMultiD { |
There was a problem hiding this comment.
Do we have an equation to reference for this? Stone 2020 Equation 5? With the caveat we're apply this to the conserveds?
There was a problem hiding this comment.
Yes, plus limited based on the AMReX implementation.
I added a pointer to both as a comment.
There was a problem hiding this comment.
I wrote that code, but I think it's correct nonetheless ;)
| const int fj = (DIM > 1) ? (j - cjb.s) * 2 + jb.s : jb.s; | ||
| const int fk = (DIM > 2) ? (k - ckb.s) * 2 + kb.s : kb.s; | ||
|
|
||
| constexpr bool INCLUDE_X1 = |
There was a problem hiding this comment.
So we include a dimension if:
- If we're prolongating to a cell centered position
- Dimensions along the plane of a face for a face element
- Dimensions along the edge for an edge element
There was a problem hiding this comment.
Yes, and to be honest, I adapted this logic from the Parthenon prolongation (without checking) trusting that it works there and we just needed limiting.
| // Normalize the thermal, kinetic, and magnetic mass fractions to sum to 1.0 | ||
| if (enable_magnetic_tower_mass_injection_) { | ||
| thermal_mass_fraction_ = thermal_fraction_; | ||
| kinetic_mass_fraction_ = kinetic_fraction_; | ||
| magnetic_mass_fraction_ = magnetic_fraction_; | ||
| } else { | ||
| const auto total_mass_frac = thermal_fraction_ + kinetic_fraction_; | ||
| thermal_mass_fraction_ = thermal_fraction_ / total_mass_frac; | ||
| kinetic_mass_fraction_ = kinetic_fraction_ / total_mass_frac; | ||
| magnetic_mass_fraction_ = 0.0; | ||
| } | ||
|
|
There was a problem hiding this comment.
In a commit I ultimately dropped, I had the mass fractions of each of the different mechanisms optionally decoupled from the energy fractions. Worth bringing back, would give a bit more functionality?
There was a problem hiding this comment.
Potentially. The only reason I decided to go for this route was that it introduces only one additional parameter in the input file.
I suggest that we make further modifications (like explicit mass fraction parameters) when needed.
| @@ -90,15 +86,16 @@ void MagneticTower::AddSrcTerm(parthenon::Real field_to_add, parthenon::Real mas | |||
| a_jb.e, a_ib.s, a_ib.e, | |||
| KOKKOS_LAMBDA(const int &b, const int &k, const int &j, const int &i) { | |||
| // Compute and apply potential | |||
| auto &A = A_pack(b); | |||
There was a problem hiding this comment.
Any particular reason to make this switch? Maybe caching that first array access into the block?
There was a problem hiding this comment.
Which switch specifically?
Previously, the potential was an explicit ParArray5D allocated an deallocated on every single call.
Now it's a standard Parthenon field that we access through the packing machinery.
Or are you referring to not using the b index and access A_pack(b,n,k,j,i) in the following lines?
In that case, I made the switch because we typically extract the block pointer also for prim and cons fields.
| a_r = 0.0; | ||
| a_theta = field_ * l_scale_ * (r / l_scale_) * exp_r2_h2; | ||
| a_h = field_ * l_scale_ * alpha_ / 2.0 * exp_r2_h2; | ||
| if (potential_ == MagneticTowerPotential::donut) { |
There was a problem hiding this comment.
I'm not sure I like this if called everytime, I'd like to optimize this out at compiler time and an if statement elsewhere.
Not necessary for this PR, however
There was a problem hiding this comment.
I'm am sure that I don't like it ;)
I just went for it for simplicity now.
I started to write a template MagneticTowerObj but then realized that we call the constructor at three places and I didn't want to add the identical logic at three places (or introduce std::variant or similar).
Therefore, I decided to keep it simple and revisit once we add even more options.
…tions Forrestglines/agn reductions
|
Merged in the cluster clips/stellar feedback reductions and cold gas/AGN extent reductions into this PR |
|
I added a couple of minor fixes (param names, prim scalar tracer for the extent (rather than the scalar density, and formatting, see 2bdf1f8...36d5139 I think this is ready for a merge (as the PR is growing, I suggest to make further modifications separately). |
|
I put up one last commit that just removes commented out code for the alternative methods to include cooling. I wanted to test this on Frontier one last time from a clean build to make sure everything is working. I got distracted trying to get ROCM 5.6.0 and ROCM 5.7.0 working but the ROCM 5.4.3 build works as expected. Merging this in. |
Summary (should all be reflected in the updated doc)