position dependent photon field for EM interactions (optimised)#535
position dependent photon field for EM interactions (optimised)#535GDMarco wants to merge 135 commits intoCRPropa:masterfrom
Conversation
//InteractionRatesIsotropic::InteractionRatesIsotropic(); //InteractionRatesIsotropic::InteractionRatesPositionDependent();
Changing nomenclature (isSpatialDependent -> isPositionDependent).
Changing nomenclature
Change nomenclature.
Change nomenclature.
style & constructor of InteractionRates subclasses
JanNiklasB
left a comment
There was a problem hiding this comment.
Hi,
thanks for your contribution!
I have done a first review of your pull request and noticed the following things.
- Please try to not include lines in your pull request that are only altered in white spaces without having any purpose, you often only change tabs to spaces or add new empty lines, this makes it difficult to spot actual changes and leads to difficult to read code.
- Reduce the amount of includes you use in your files, any not needed include or already in the header existing includes should not be in the .cpp file again. This is only a style thing since otherwise the code seems a little cluttered.
- For the 4 changed module files (EM*.h and EM*.cpp) I only commented in EMDoublePairProduction.* but my comments hold true for all module files:
-> The old behavior of function and classes regarding function calls can not be changed, it needs to be backwards compatible ( We need to at least notify the community first when we remove or change crucial features)
-> Everything needs to be as modular as possible, so if you use a class like the interaction rates in a function you need to only require the base class so the user can create their own versions and hand them over. Those base classes then need everything to distinguish them inside those functions - You require the function
splitFilenamein many of your classes but this function does not depend on any class member, so it would be better to add this function to Common.h as a standalone - Every function which is not obvious to use or where it is not obvious what it does needs to have a description
- You need to add your changes to the change log
CHANGELOG.md
Many comments would repeat themself so I usually mention something only 1 or 2 times.
With regards,
Jan-Niklas
| std::string getInteractionTag() const; | ||
|
|
||
| void initRate(std::string filename, InteractionRatesHomogeneous* intRatesHom); | ||
| void initRatePositionDependentPhotonField(std::string filepath, InteractionRatesPositionDependent* intRatesPosDep); |
There was a problem hiding this comment.
A description of initRate and initRatePositionDependentPhotonField is necessary, it is not clear for the average user how to use those functions.
Furthermore, we might need to add a initRate(std::string) overload so the old call behavior is preserverd.
There was a problem hiding this comment.
Thanks for the comment! I put a brief description of the init*Rate functions, although the user do not need to handle those directly.
Could you please elaborate further the second part of this comment?
There was a problem hiding this comment.
Originally initRate was callable with the signiture initRate(std::string), this still needs to be possible, otherwise existing simulations will fail.
| if (!this->photonField->hasPositionDependence()) { | ||
|
|
||
| this->interactionRates = new InteractionRatesHomogeneous("interactionRatesHomogeneous", false); | ||
| InteractionRatesHomogeneous* intRatesHom = static_cast<InteractionRatesHomogeneous*>(this->interactionRates.get()); |
There was a problem hiding this comment.
With the requested change of initRate in the header file this pointer and the one in line 51 are not required. Furthermore initRate would be used in both cases so the if condition would is then only required inside the initRate function.
CMakeLists.txt
Outdated
|
|
||
| endif(ENABLE_PYTHON AND Python_FOUND) | ||
|
|
||
| target_include_directories(crpropa |
There was a problem hiding this comment.
Please add nanoflann to crpropa over CRPROPA_EXTRA_INCLUDE and CRPROPA_EXTRA_SOURCES (see other libraries).
There was a problem hiding this comment.
I add it to the CRPROPA_EXTRA_INCLUDE, as the other libraries (e.g. kiss). Why also to CRPROPA_EXTRA_SOURCES?
There was a problem hiding this comment.
You are right, for nanoflann you only have Includes, so only to CRPROPA_EXTRA_INCLUDE
|
Thanks for the comments! I fixed and commented all the issues you raised. I will need to implement it merging the currently accepted PRs. |
-> now handled internally and only casted in init*Rates functions
…r to interactions
Changes regarding the PR#535
JanNiklasB
left a comment
There was a problem hiding this comment.
Hey,
sorry it took me a little bit longer.
I found a very small typo in one of the example files and encountered a missing install statement in the CMakeLists.txt which I missed last time (sorry)
Other then that I think everything looks good!
| "Acon = 0.1\n", | ||
| "\n", | ||
| "# radius of the spherical observer placed at the Earth position (kpc).\n", | ||
| "Orad = np.tan(Scon) * D \n", |
There was a problem hiding this comment.
I think Scon should be Acon since Scon is nowhere defined
| "ebl = IRB_Gilmore12()\n", | ||
| "\n", | ||
| "# position dependent. (Sphere to load only the ISRF nodes close to observer and the source.)\n", | ||
| "isrf = ISRF_Freudenreich98(Sphere(Vector3d(x, y, z) * kpc, D * kpc + 1 * kpc))" |
There was a problem hiding this comment.
This is currently failing, but only because of the currently missing photon files so it is fine (fyi @Grindegreen )
| install(DIRECTORY ${CMAKE_BINARY_DIR}/include/ DESTINATION include FILES_MATCHING PATTERN "*.h") | ||
| install(DIRECTORY ${CMAKE_BINARY_DIR}/data/ DESTINATION share/crpropa/ PATTERN ".git" EXCLUDE) | ||
| install(DIRECTORY libs/kiss/include/ DESTINATION include) | ||
|
|
There was a problem hiding this comment.
please add install(DIRECTORY libs/nanoflann/include/ DESTINATION include) so the nanoflann header files are included correctly (I missed that last time, sorry)
General description
This is a fully working version of the CRPropa code with some implementation to allow spatially dependent photon field to be initialised and employed for electromagnetic interactions. A particular example is the introduction of the galactic interstellar field (the density and interaction rate tables are at this repo: ISRF tables.)
In principle, this implementation can be used for any spatial dependent photon field properly build by the user.
To allow the introduction of such spatially variable photon fields in CRPropa, the classe and modules that have been modified are
PhotonBackground.*,EMPairProduction.*,EMDoublePairProduction.*,EMInverseComptonScattering.*andEMTripletPairProduction.*. Moreover, the classInteractionRates.*has been implemented to handle the process rates.An example on how to use the code is contained in the notebooks.
Technical details
The
.ccfiles ofPhotonBackground.*,EMPairProduction.*,EMDoublePairProduction.*,EMInverseComptonScattering.*andEMTripletPairProduction.*employ some tools of the#include <filesystem>library, only available sinceC++17. The proper library is specified in theCMakeLists.txtfiles, as well as in the header of the.ccfiles.The code uses the nanoflann functionalities to look for the closest photon field point in your custom grid and evaluate the density and/or the rate correctly. In this new version, nanoflann is implemented as a header-only library among the ones in the
/libs/directory.