search

CRPropa / CRPropa3

CRPropa is a public astrophysical simulation framework for propagating extraterrestrial ultra-high energy particles. https://crpropa.github.io/CRPropa3/
https://crpropa.desy.de
GNU General Public License v3.0
65 stars 66 forks source link

Saldana-Lopez 2021 EBL model #434

Closed IevgenVovk closed 9 months ago

IevgenVovk commented 1 year ago

This PR adds the recent Saldana-Lopez 2021 EBL model - along with its lower and upper limits - to CRPropa. The model data files are part of the related PR to the CRPropa-data repository.

rafaelab commented 1 year ago

Thanks for the PR, @IevgenVovk. I will test it in ~2 weeks.

For now, I just have one minor comment. Since "Saldaña-Lopez" is a composite surname, perhaps the model could be named IRB_SaldanaLopez21 instead of IRB_Saldana21. To me it makes no difference at all , so the choice would ultimately depend on the preference of the author of the EBL model (who might just not care about it). What I can say is that, as someone with two surnames, I do prefer to be cited with both of them instead of just one.

IevgenVovk commented 1 year ago

Thanks for the comment, @rafaelab . IRB_SaldanaLopez21 was my preference too, but then I've noticed the data files shared by the authors are called *saldana21*; I've then decided to follow their naming.

rafaelab commented 1 year ago

Perfect, @IevgenVovk . I didn't know that, so I think that settles the issue and we keep the current names :)

rafaelab commented 1 year ago

Hi @IevgenVovk Everything looks good to me. Could you just add to CHANGELOG.md, under "New features", the information that you added IRB_Saldana21 EBL model?

IevgenVovk commented 12 months ago

Hi @rafaelab , thanks for checking. Sure, have updated the CHANGELOG.md

lukasmerten commented 9 months ago

I guess it would be good to include the new model into the default CRPropa models. That means we have to update the default data that is downloaded during the installation process.

I will run the necessary scripts of CRPropa-data and edit this PR accordingly. Afterwards we can approve and merge.

lukasmerten commented 9 months ago

I have updated the data download to a new data package that includes the Saldana 21 data. This is now also reflected in an update to the CRPropa-Data repository (https://github.com/CRPropa/CRPropa3-data/pull/19).

rafaelab commented 9 months ago

Thanks for the contribution, @IevgenVovk. It works just fine with me.

I have also double checked the CRPropa-data repository modified by @lukasmerten, just to make sure there are no inconsistencies.

I think we can merge this and just open another PR to fix the minor inconsistency in the information displayed when running CMake, as pointed by @JulienDoerner. I don't suppose Ievgen will have this information (like file size) to implement the change himself.

lukasmerten commented 9 months ago

I‘ll update it later today. Thanks for taking a look at it.