-
Notifications
You must be signed in to change notification settings - Fork 127
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
Add non-relativistic radiation transport solvers (explicit and implicit) and cosmic ray transport #492
Conversation
@felker How to add equations in the documentation easily. |
I am working on merging the chemistry_scalar branch, which should be done in the next ~2 weeks before the Athena++ conference. There is also a radiation class there, although it's mostly just a container. See src/radiation on the branch. |
@yanfeij I just learned that as of very recently, github brought back mathjax support! So you can enclose basic latex commands in |
I am afraid it will have exactly the same name as what I am using. Looks like you are just following six fixed rays, which can be a very special case of general radiation transport. |
It works! Thank you. |
@yanfeij Yes, the six-ray integrator is a very special case. I am hoping the container Radiation class is general enough to be shared among different radiation modules. Perhaps we should discuss this. See src/radiation/radiation.hpp (or.cpp) in chemistry_scalar branch. |
There are many more steps to do for general radiation transport. The implicit radiation transport is also using a completely different integrator. The data array structure and boundary values I need are also different. You can also take a look at the rad_newtonian branch to get an idea. |
Just as a heads up, after this is merged I will work on merging the Maybe we should decide among the following cases soon:
|
The two modules I have share some common data structures. But I think it will be a lot of work to make all these "radiation" to use the same integrator or data arrays. Also we do not expect to turn on any two of these all at once. I think it is easier to keep each one separate. But do give them some specific names, not just radiation. For example, six_ray_tracing, gr_radiation or something like that. |
… into rad_newtonian merge remote changes
OK. My branch has a long history.
…On Tue, Jul 11, 2023 at 11:55 AM Kyle Gerard Felker < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
On tst/regression/scripts/tests/shearingbox/mri3d.py
<#492 (comment)>
:
looks like a dirty merge from master. @tomo-ono
<https://github.com/tomo-ono> removed the test in Dec 2020 in 8271040
<8271040>
(not sure why though?)
We should also remove
https://github.com/PrincetonUniversity/athena/blob/rad_newtonian/tst/regression/data/mhd_mri_3d.hst
too
—
Reply to this email directly, view it on GitHub
<#492 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCWEJYIZV2DFJMDBJONPVTXPTFCBANCNFSM6AAAAAAW5P23YY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Best,
Yan-Fei Jiang
Associate Research Scientist,
Flatiron Institute,
160 Fifth Avenue
New York, NY 10010
|
@tomidakn might get a chance to take a look at this PR on Wednesday--- if not, we merge immediately on Thursday |
retest this |
I've reviewed about half. Will do the rest tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the huge effort. I quickly reviewed the code, and I just have some comments, mostly cosmetic. I think some files (matrix utils) can be merged, and also some function names can be more self-explanatory.
src/nr_radiation/frequencygrid.cpp
Outdated
// frequency is scaled with kT_0/h | ||
// using fitting formula to return \int_0^nu_min and \int_0^nu_max | ||
|
||
Real NRRadiation::FitBlackBody(Real nu_t) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function names "FitBlackBody" and "BlackBodySpec" sound confusing to me. I prefer to explicitly indicate that they are integration of the Planck function.
}// End (RADIATION_ENABLED) | ||
|
||
|
||
if (CR_ENABLED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about "adding cr" data set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you clarify? like another user option for the output block like outputN/variable=cr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It may convenient to group CR data sets (as well as radiation data sets).
src/task_list/im_rad_task_list.hpp
Outdated
struct IMRadTask { | ||
TaskID task_id; //!> encodes task using bit positions in IMRadTaskNames | ||
TaskID dependency; //!> encodes dependencies to other tasks using IMRadTaskNames | ||
TaskStatus (IMRadTaskList::*TaskFunc)(MeshBlock *); //!> ptr to a task | ||
}; | ||
|
||
//---------------------------------------------------------------------------------------- | ||
//! \class MultigridTaskList | ||
//! \brief data and function definitions for IMRadTaskList class | ||
|
||
class IMRadTaskList { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we derive the TaskList class instead of redefining these codes? Also, the comment must be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that MultigridTaskList
is not derived either?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but it is intentional for a few reasons. MG works on Multigrid objects, not MeshBlocks. This is for future extension to apply different parallelizations for MG and the main part. Maybe I can still derive it from the TaskList, though.
For this radiation, I think it can be done similarly to STS Task List.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the IMRadTaskList
has a similar argument as the MultigridTaskList
then, since the function signature is also different. It does not have a notion of a time integrator stage:
TaskStatus (IMRadTaskList::*TaskFunc)(MeshBlock *); //!> ptr to a task
The SuperTimeStepTaskList
does use int stage
in a similar fashion to the main task list, so it isnt quite the same as the radiation task list.
Anyway, let's continue this discussion on #537
|
||
|
||
//-------------------------------------------------------------------------------------- | ||
//! \fn void Gauleg(int n, Real x1, Real x2, AthenaArray<Real> &x, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not severely limit the length of function names. I think it is better to use names that are not shortened in general. For example, GaussLegendre().
… into rad_newtonian merge remote changes
@yanfeij, when you have time, can you add some practical usage of the radiation transport and CR transport modules in the Wiki, including outputs and analyses? For example, Brief description of the test codes is helpful for users. |
Will do after my travel this week.
…On Tue, Jul 18, 2023 at 11:55 AM Kengo TOMIDA ***@***.***> wrote:
@yanfeij <https://github.com/yanfeij>, when you have time, can you add
some practical usage of the radiation transport and CR transport modules in
the Wiki, including outputs and analyses? For example, Brief description of
the test codes is helpful for users.
—
Reply to this email directly, view it on GitHub
<#492 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCWEJ3XPZCO5SDAOZAUMB3XQYCMFANCNFSM6AAAAAAW5P23YY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Best,
Yan-Fei Jiang
Associate Research Scientist,
Flatiron Institute,
160 Fifth Avenue
New York, NY 10010
|
Thanks!! |
Just opening this draft PR to start parsing the massive amount of changes and create an area for discussion.
tst/multi_group/
needs to be reorganized and cleaned up..ipynb
notebooks to version control; do we want to do that?athena_read.py