Skip to content
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

Merged
merged 155 commits into from
Jul 16, 2023

Conversation

felker
Copy link
Contributor

@felker felker commented Apr 13, 2023

Just opening this draft PR to start parsing the massive amount of changes and create an area for discussion.

CHANGELOG.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Makefile.in Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
configure.py Outdated Show resolved Hide resolved
configure.py Outdated Show resolved Hide resolved
@yanfeij
Copy link
Contributor

yanfeij commented Apr 14, 2023

@felker How to add equations in the documentation easily.

@munan
Copy link
Contributor

munan commented Apr 14, 2023

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.

@c-white
Copy link
Contributor

c-white commented Apr 14, 2023

@felker How to add equations in the documentation easily.

@yanfeij I just learned that as of very recently, github brought back mathjax support! So you can enclose basic latex commands in $...$ or $$...$$ and it should work.

@yanfeij
Copy link
Contributor

yanfeij commented Apr 14, 2023

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.

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.

@yanfeij
Copy link
Contributor

yanfeij commented Apr 14, 2023

@felker How to add equations in the documentation easily.

@yanfeij I just learned that as of very recently, github brought back mathjax support! So you can enclose basic latex commands in $...$ or $$...$$ and it should work.

It works! Thank you.

@munan
Copy link
Contributor

munan commented Apr 14, 2023

@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.

@yanfeij
Copy link
Contributor

yanfeij commented Apr 14, 2023

@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.

@c-white
Copy link
Contributor

c-white commented Apr 14, 2023

Just as a heads up, after this is merged I will work on merging the gr_rad branch as well. This will probably be more difficult, as there is a lot of overlap in files and functions but not details of what the code is doing.

Maybe we should decide among the following cases soon:

  1. All radiation -- whether written by @munan or @yanfeij or me or anyone else) will eventually live in the same radiation module (i.e., the same class Radiation and the same directory src/radiation/). This might make for a confusingly large module.
  2. Each radiation implementation will be separate, unless they already coexist (maybe some of @yanfeij 's do). There might be some duplicated code, but at least those interested in one use case would not have to sift through code pertaining to vastly different cases.
  3. Something in between.

@yanfeij
Copy link
Contributor

yanfeij commented Apr 14, 2023

Just as a heads up, after this is merged I will work on merging the gr_rad branch as well. This will probably be more difficult, as there is a lot of overlap in files and functions but not details of what the code is doing.

Maybe we should decide among the following cases soon:

  1. All radiation -- whether written by @munan or @yanfeij or me or anyone else) will eventually live in the same radiation module (i.e., the same class Radiation and the same directory src/radiation/). This might make for a confusingly large module.
  2. Each radiation implementation will be separate, unless they already coexist (maybe some of @yanfeij 's do). There might be some duplicated code, but at least those interested in one use case would not have to sift through code pertaining to vastly different cases.
  3. Something in between.

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.

@yanfeij
Copy link
Contributor

yanfeij commented Jul 11, 2023 via email

@felker
Copy link
Contributor Author

felker commented Jul 11, 2023

@tomidakn might get a chance to take a look at this PR on Wednesday--- if not, we merge immediately on Thursday

@felker
Copy link
Contributor Author

felker commented Jul 12, 2023

retest this

@tomidakn
Copy link
Contributor

I've reviewed about half. Will do the rest tomorrow.

Copy link
Contributor

@tomidakn tomidakn left a 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/coordinates/cylindrical.cpp Outdated Show resolved Hide resolved
src/coordinates/spherical_polar.cpp Outdated Show resolved Hide resolved
// 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) {
Copy link
Contributor

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.

src/nr_radiation/implicit/rad_iteration.cpp Outdated Show resolved Hide resolved
src/nr_radiation/integrators/rad_source.cpp Outdated Show resolved Hide resolved
}// End (RADIATION_ENABLED)


if (CR_ENABLED) {
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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).

Comment on lines 32 to 42
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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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,
Copy link
Contributor

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().

src/utils/inverse_matrix.cpp Outdated Show resolved Hide resolved
src/utils/inverse_matrix.cpp Outdated Show resolved Hide resolved
@felker felker merged commit 5a59818 into master Jul 16, 2023
1 check passed
@felker felker deleted the rad_newtonian branch July 16, 2023 15:47
@tomidakn
Copy link
Contributor

@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.

@yanfeij
Copy link
Contributor

yanfeij commented Jul 18, 2023 via email

@tomidakn
Copy link
Contributor

Will do after my travel this week.

Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants