AGN triggering_inflow rate #133
Conversation
compute Inflow rate for total mass and cold gas mass
modified initialization for inflow rate
There was a problem hiding this comment.
Thanks for reporting this way.
This made looking a the code much easier.
I left a couple of general comments.
The compilation errors you see are like tied to trying to modify the member variables of a (const) object extracted from Params.
If I followed the code correctly, you don't even need those member variables updating the code in general should also fix the compilation issue.
| parthenon::MeshData<parthenon::Real> *md, | ||
| const parthenon::Real dt, const EOS eos) const { |
There was a problem hiding this comment.
I think the const can stay as the function does not modify the member variables of the AGNTriggering object.
src/pgen/cluster/agn_triggering.cpp
Outdated
| auto &cons = cons_pack(b); | ||
| auto &prim = prim_pack(b); | ||
| const auto &coords = cons_pack.GetCoords(b); | ||
|
|
||
| const parthenon::Real r2 = | ||
| pow(coords.Xc<1>(i), 2) + pow(coords.Xc<2>(j), 2) + pow(coords.Xc<3>(k), 2); | ||
| if (r2 < accretion_radius2) { | ||
|
|
||
| const Real cell_total_mass = prim(IDN, k, j, i) * coords.CellVolume(k, j, i); | ||
| team_total_mass += cell_total_mass; |
There was a problem hiding this comment.
As for the team_cold_mass below, you should only account for the gas in the interior part (not within the ghost zones).
For reference, we remove the accreted gas in all cells (also within the ghost zones) because there's not boundary exchange afterwards so all the cells need to be consistent in all blocks.
| if (k >= kb.s && k <= kb.e && j >= jb.s && j <= jb.e && i >= ib.s && i <= ib.e) { | ||
| auto &prim = prim_pack(b); | ||
| const auto &coords = prim_pack.GetCoords(b); | ||
| const parthenon::Real r2 = | ||
| pow(coords.Xc<1>(i), 2) + pow(coords.Xc<2>(j), 2) + pow(coords.Xc<3>(k), 2); | ||
| if (r2 < accretion_radius2) { | ||
| const Real cell_mass = prim(IDN, k, j, i) * coords.CellVolume(k, j, i); | ||
| if (r2 < accretion_radius2) { | ||
| const Real cell_mass = prim(IDN, k, j, i) * coords.CellVolume(k, j, i); | ||
|
|
||
| const Real cell_mass_weighted_density = cell_mass * prim(IDN, k, j, i); | ||
| const Real cell_mass_weighted_velocity = | ||
| const Real cell_mass_weighted_density = cell_mass * prim(IDN, k, j, i); | ||
| const Real cell_mass_weighted_velocity = | ||
| cell_mass * sqrt(pow(prim(IV1, k, j, i), 2) + pow(prim(IV2, k, j, i), 2) + | ||
| pow(prim(IV3, k, j, i), 2)); | ||
| const Real cell_mass_weighted_cs = | ||
| const Real cell_mass_weighted_cs = | ||
| cell_mass * sqrt(gamma * prim(IPR, k, j, i) / prim(IDN, k, j, i)); | ||
|
|
||
| ltotal_mass_red += cell_mass; | ||
| lmass_weighted_density_red += cell_mass_weighted_density; | ||
| lmass_weighted_velocity_red += cell_mass_weighted_velocity; | ||
| lmass_weighted_cs_red += cell_mass_weighted_cs; | ||
| } | ||
| ltotal_mass_red += cell_mass; | ||
| lmass_weighted_density_red += cell_mass_weighted_density; | ||
| lmass_weighted_velocity_red += cell_mass_weighted_velocity; | ||
| lmass_weighted_cs_red += cell_mass_weighted_cs; | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
What's going on with these changes here?
As far as I can tell the if k>=kb.s is new, but it does't change anything as the indices only go over the interior indices anyway.
There was a problem hiding this comment.
I think this ia a bug because I didn't change this part of the code
src/pgen/cluster/agn_triggering.cpp
Outdated
| @@ -361,16 +382,20 @@ AGNTriggering::GetAccretionRate(parthenon::StateDescriptor *hydro_pkg) const { | |||
| return 0; | |||
| } | |||
| } | |||
| return 0; | |||
| //return 0; | |||
There was a problem hiding this comment.
I think we should keep returning 0 by default.
src/pgen/cluster/agn_triggering.cpp
Outdated
| @@ -393,24 +418,28 @@ AGNTriggeringReduceTriggering(parthenon::MeshData<parthenon::Real> *md, | |||
| const parthenon::Real dt) { | |||
|
|
|||
| auto hydro_pkg = md->GetBlockData(0)->GetBlockPointer()->packages.Get("Hydro"); | |||
| const auto &agn_triggering = hydro_pkg->Param<AGNTriggering>("agn_triggering"); | |||
|
|
|||
| auto &agn_triggering = hydro_pkg->Param<AGNTriggering>("agn_triggering"); | |||
There was a problem hiding this comment.
I think this can stay const.
src/pgen/cluster/agn_triggering.cpp
Outdated
| if (agn_triggering.triggering_mode_ == AGNTriggeringMode::COLD_GAS) { | ||
| auto &agn_triggering = hydro_pkg->Param<AGNTriggering>("agn_triggering"); // Make it non-const | ||
| const parthenon::Real cold_mass = hydro_pkg->Param<Real>("agn_triggering_cold_mass"); | ||
| const parthenon::Real total_mass = hydro_pkg->Param<Real>("agn_triggering_total_mass"); | ||
|
|
||
| hydro_pkg->UpdateParam<Real>("agn_triggering_prev_cold_mass", cold_mass); //Move from AGNTriggeringFinalizeTriggering | ||
| hydro_pkg->UpdateParam<Real>("agn_triggering_prev_total_mass", total_mass); | ||
| } | ||
| hydro_pkg->UpdateParam<AGNTriggering>("agn_triggering", agn_triggering); |
There was a problem hiding this comment.
What's the intent here?
As far as I can tell you're already updating the prev values above.
src/pgen/cluster/agn_triggering.cpp
Outdated
| Real accretion_rate = hydro_pkg->Param<Real>("agn_triggering_cold_mass"); | ||
| PARTHENON_MPI_CHECK(MPI_Allreduce(MPI_IN_PLACE, &accretion_rate, 1, | ||
| MPI_PARTHENON_REAL, MPI_SUM, MPI_COMM_WORLD)); | ||
| hydro_pkg->UpdateParam("agn_triggering_cold_mass", accretion_rate); | ||
|
|
||
| Real total_mass = hydro_pkg->Param<Real>("agn_triggering_total_mass"); | ||
| PARTHENON_MPI_CHECK(MPI_Allreduce(MPI_IN_PLACE, &total_mass, 1, | ||
| MPI_PARTHENON_REAL, MPI_SUM, MPI_COMM_WORLD)); | ||
| hydro_pkg->UpdateParam("agn_triggering_total_mass", total_mass); |
There was a problem hiding this comment.
Given that MPI_Allreduce are expensive operations, I suggest to combine both numbers, c.f.,
athenapk/src/pgen/turbulence.cpp
Line 384 in f8497c5
src/pgen/cluster/agn_triggering.cpp
Outdated
| //return std::numeric_limits<Real>::max(); | ||
| } | ||
|
|
||
| } // namespace cluster | ||
|
|
There was a problem hiding this comment.
Why these changes? Does this compile?
src/pgen/cluster/agn_triggering.hpp
Outdated
|
|
||
| parthenon::Real inflow_cold_=0.0; // Cold mass inflow rate | ||
| parthenon::Real inflow_tot_=0.0; // Total gas mass inflow rate |
There was a problem hiding this comment.
Why do you need those new variables?
src/pgen/cluster/agn_triggering.cpp
Outdated
| agn_triggering.inflow_cold_ = (cold_mass - hydro_pkg->Param<Real>("agn_triggering_prev_cold_mass")) / tm.dt; //Now this is the only place to calculate inflow rates | ||
| agn_triggering.inflow_tot_ = (total_mass - hydro_pkg->Param<Real>("agn_triggering_prev_total_mass")) / tm.dt; //Now this is the only place to calculate inflow rates | ||
|
|
There was a problem hiding this comment.
Why are those new member variables required?
As far as I can tell these are temporary values only used in this function as you can calculate those from total and prev_totals
There was a problem hiding this comment.
because I need to compute the inflow rate and in Finalizetriggering function I compute inflow rate values and record the values in order to get them in output in agn_triggering.dat file (line 553 554)
|
I changed the file and now the error is: ```[ 88%] Building CXX object src/CMakeFiles/athenaPK.dir/pgen/cluster/cluster_clips.cpp.o /home/elyon/athenapk/external/parthenon/src/kokkos_abstraction.hpp(696): error: no instance of overloaded function "parthenon::par_dispatch" matches the argument list 2 errors detected in the compilation of "/home/elyon/athenapk/src/pgen/cluster/agn_triggering.cpp". |
Is this the full error log? I'm having trouble matching the error to the changed code I'm currently seeing. |
|
yes this is the full error log and before re-compiling I changed the code
and then typed 'make clean' and 'make'.
…On Fri, 3 Jan 2025 at 14:31, Philipp Grete ***@***.***> wrote:
I changed the file and now the error is: ```[ 88%] Building CXX object
src/CMakeFiles/athenaPK.dir/pgen/cluster/cluster_clips.cpp.o
/home/elyon/athenapk/external/parthenon/src/kokkos_abstraction.hpp(696):
error: no instance of overloaded function "parthenon::par_dispatch" matches
the argument list argument types are: (parthenon::LoopPatternMDRange, const
char [29], parthenon::DevExecSpace, int, int, int, int, int, int, int, int,
lambda [](const int &, const int &, const int &, const int &,
parthenon::Real &, parthenon::Real &)->void, Kokkos::Sum<double,
Kokkos::Serial::memory_space>, Kokkos::Sum<double,
Kokkos::Serial::memory_space>)
par_dispatch<dispatch_impl::ParallelReduceDispatch>(std::forward(args)...);
^ detected during: instantiation of "void parthenon::par_reduce(Args &&...)
[with Args=<parthenon::LoopPatternMDRange &, const char (&)[29],
parthenon::DevExecSpace, int, int, int &, int &, int &, int &, int &, int
&, lambda [](const int &, const int &, const int &, const int &,
parthenon::Real &, parthenon::Real &)->void, Kokkos::Sum<double,
Kokkos::Serial::memory_space>, Kokkos::Sum<double,
Kokkos::Serial::memory_space>>]" at line 216 of
/home/elyon/athenapk/src/pgen/cluster/agn_triggering.cpp instantiation of
"void cluster::AGNTriggering::ReduceColdMass(parthenon::Real &,
parthenon::Real &, parthenon::MeshDataparthenon::Real *, parthenon::Real,
EOS) const [with EOS=AdiabaticHydroEOS]" at line 435 of
/home/elyon/athenapk/src/pgen/cluster/agn_triggering.cpp
/home/elyon/athenapk/external/parthenon/src/kokkos_abstraction.hpp(696):
error: no instance of overloaded function "parthenon::par_dispatch" matches
the argument list argument types are: (parthenon::LoopPatternMDRange, const
char [29], parthenon::DevExecSpace, int, int, int, int, int, int, int, int,
lambda [](const int &, const int &, const int &, const int &,
parthenon::Real &, parthenon::Real &)->void, Kokkos::Sum<double,
Kokkos::Serial::memory_space>, Kokkos::Sum<double,
Kokkos::Serial::memory_space>)
par_dispatch<dispatch_impl::ParallelReduceDispatch>(std::forward(args)...);
^ detected during: instantiation of "void parthenon::par_reduce(Args &&...)
[with Args=<parthenon::LoopPatternMDRange &, const char (&)[29],
parthenon::DevExecSpace, int, int, int &, int &, int &, int &, int &, int
&, lambda [](const int &, const int &, const int &, const int &,
parthenon::Real &, parthenon::Real &)->void, Kokkos::Sum<double,
Kokkos::Serial::memory_space>, Kokkos::Sum<double,
Kokkos::Serial::memory_space>>]" at line 216 of
/home/elyon/athenapk/src/pgen/cluster/agn_triggering.cpp instantiation of
"void cluster::AGNTriggering::ReduceColdMass(parthenon::Real &,
parthenon::Real &, parthenon::MeshDataparthenon::Real *, parthenon::Real,
EOS) const [with EOS=AdiabaticGLMMHDEOS]" at line 438 of
/home/elyon/athenapk/src/pgen/cluster/agn_triggering.cpp
2 errors detected in the compilation of
"/home/elyon/athenapk/src/pgen/cluster/agn_triggering.cpp". make[2]: ***
[src/CMakeFiles/athenaPK.dir/build.make:342:
src/CMakeFiles/athenaPK.dir/pgen/cluster/agn_triggering.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs.... make[1]: ***
[CMakeFiles/Makefile2:1749: src/CMakeFiles/athenaPK.dir/all] Error 2 make:
*** [Makefile:146: all] Error 2```
Is this the full error log? I'm having trouble matching the error to the
changed code I'm currently seeing.
Also, is this a clean build or a rebuild from a previous version with some
cached objects?
—
Reply to this email directly, view it on GitHub
<#133 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BNYBFSZJU5C25TXDRDJN55T2I2GJFAVCNFSM6AAAAABT3KWP7WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRZGIZDKOBVGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
To get a cleaner error message, I suggest to compile with a single process (so that multiple messages are not printed in between). Note that the line causing the error (line 171) was not included in the error you posted above (which made it difficult to track this down). |
src/pgen/cluster/agn_triggering.cpp
Outdated
|
|
||
|
|
||
| parthenon::par_reduce( | ||
| parthenon::loop_pattern_mdrange_tag, "AGNTriggering::ReduceColdGas", | ||
| parthenon::DevExecSpace(), 0, cons_pack.GetDim(5) - 1, kb.s, kb.e, jb.s, jb.e, ib.s, | ||
| ib.e, | ||
| KOKKOS_LAMBDA(const int &b, const int &k, const int &j, const int &i, | ||
| Real &team_cold_mass) { | ||
| Real &team_cold_mass, Real &team_total_mass) { |
There was a problem hiding this comment.
Looks like the abstraction for a par reduce to two scalars is currently not available within Parthenon (though we're working on this).
In the meantime I suggest to use a raw Kokkos pattern, i.e.,
Kokkos::parallel_reduce(
"AGNTriggering::ReduceColdGas",
Kokkos::MDRangePolicy<Kokkos::Rank<4>>(
DevExecSpace(), {0, kb.s, jb.s, ib.s},
{cons_pack.GetDim(5), kb.e + 1, jb.e + 1, ib.e + 1},
{1, 1, 1, ib.e + 1 - ib.s}),
KOKKOS_LAMBDA(const int &b, const int &k, const int &j, const int &i,
Real &team_cold_mass, Real &team_total_mass) {
src/pgen/cluster/agn_triggering.cpp
Outdated
| inflow_tot = (total_mass - hydro_pkg->Param<Real>("agn_triggering_prev_total_mass")) / tm.dt; | ||
|
|
||
| } | ||
| hydro_pkg->UpdateParam<AGNTriggering>("agn_triggering", agn_triggering); |
There was a problem hiding this comment.
This line can probably go as it's not required (agn_triggering is not modified in the code above) and might raise an error because you're updating a non-mutable object.
Updated function Reducecoldgas with Kokkos parralel_reduce intead of Parthenon
deleted the updating of agn_triggering
|
Removing the line agn_triggering update param, I got the same error when I
try to run the simulation:
--------------------------------------------------------------------
terminate called after throwing an instance of 'std::runtime_error'
what(): ### PARTHENON ERROR
Condition: static_cast<bool>(myMutable_.at(key))
Message: Parameter agn_triggering must be marked as mutable
File:
/scratch1/epanini/athenapk/external/parthenon/src/interface/params.hpp
Line number: 70
[gnode01:2396205] *** Process received signal ***
[gnode01:2396205] Signal: Aborted (6)
[gnode01:2396205] Signal code: (-6)
[gnode01:2396205] [ 0] /usr/lib64/libpthread.so.0(+0x12cf0)[0x154757dbccf0]
[gnode01:2396205] [ 1] /usr/lib64/libc.so.6(gsignal+0x10f)[0x154757103aff]
[gnode01:2396205] [ 2] /usr/lib64/libc.so.6(abort+0x127)[0x1547570d6ea5]
[gnode01:2396205] [ 3] /usr/lib64/libstdc++.so.6(+0x9009b)[0x154757aa509b]
[gnode01:2396205] [ 4] /usr/lib64/libstdc++.so.6(+0x9653c)[0x154757aab53c]
[gnode01:2396205] [ 5] /usr/lib64/libstdc++.so.6(+0x96597)[0x154757aab597]
[gnode01:2396205] [ 6] /usr/lib64/libstdc++.so.6(+0x9652e)[0x154757aab52e]
[gnode01:2396205] [ 7] ./bin/athenaPK[0xb97dc3]
[gnode01:2396205] [ 8] ./bin/athenaPK[0x4cc7ca]
[gnode01:2396205] [ 9] ./bin/athenaPK[0x9dd326]
[gnode01:2396205] [10] ./bin/athenaPK[0x44da57]
[gnode01:2396205] [11]
/usr/lib64/libc.so.6(__libc_start_main+0xe5)[0x1547570efd85]
[gnode01:2396205] [12] ./bin/athenaPK[0x4c54ce]
[gnode01:2396205] *** End of error message ***
/var/spool/slurm/job54550/slurm_script: line 20: 2396205 Aborted
(core dumped) ./bin/athenaPK -i ../inpu
ts/cluster/clus_nostar.in parthenon/time/tlim=1 -d
/scratch1/epanini/athenapk/build-gpu/data
…On Tue, 7 Jan 2025 at 09:52, Philipp Grete ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/pgen/cluster/agn_triggering.cpp
<#133 (comment)>
:
> - const auto &agn_triggering = hydro_pkg->Param<AGNTriggering>("agn_triggering");
+ auto &agn_triggering = hydro_pkg->Param<AGNTriggering>("agn_triggering");
+ parthenon::Real cold_mass = 0.0;
+ parthenon::Real total_mass = 0.0;
+ parthenon::Real inflow_cold = 0.0;
+ parthenon::Real inflow_tot = 0.0;
+ if (agn_triggering.triggering_mode_ == AGNTriggeringMode::COLD_GAS) {
+ cold_mass = hydro_pkg->Param<Real>("agn_triggering_cold_mass");
+ total_mass = hydro_pkg->Param<Real>("agn_triggering_total_mass");
+
+
+ inflow_cold = (cold_mass - hydro_pkg->Param<Real>("agn_triggering_prev_cold_mass")) / tm.dt;
+ inflow_tot = (total_mass - hydro_pkg->Param<Real>("agn_triggering_prev_total_mass")) / tm.dt;
+
+ }
+ hydro_pkg->UpdateParam<AGNTriggering>("agn_triggering", agn_triggering);
This line can probably go as it's not required (agn_triggering is not
modified in the code above) and might raise an error because you're
updating a non-mutable object.
—
Reply to this email directly, view it on GitHub
<#133 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BNYBFS3T35HWVBYHGK4TH7T2JOIV7AVCNFSM6AAAAABT3KWP7WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMZTG4ZDCNZWGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I currently don't see how this could be happening as there's no "Update" call to |
The objective is compute the inflow rate for total gas mass (total mass[i]-total mass[i-1])/(time[i]-time[i-1]) and inflow rate cold for only cold mass (cold mass[i]-cold mass[i-1])/(time[i]-time[i-1]) and output the values in agn_triggering.dat together accretion rate and others. When I compile the code I got some errors about initializing variables and their call/update, because some parameters are constant also if I remove 'const' definition.