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

Default behaviour of outputs #428

Closed rafaelab closed 9 months ago

rafaelab commented 1 year ago

Hi all,

When the CandidateTagColumn was added, for some reason it also became the default to add it to the output automatically. There is no reason for this to be the default. It makes the output larger, slows down the speed of output writing (even if this is negligible), and many people don't require it. Moreover, this breaks down scripts of many users for TextOutput. I propose to revert to the previous behaviour before the next release, and leave this field as an option that can be enabled/disabled. In fact, I think the default output should remain unchanged unless there is a reason to do so. Not even the weights which are necessary for electromagnetic cascades are added to the output by default.

@JulienDoerner : you were the one who implemented this, right? What is your opinion?

JulienDoerner commented 1 year ago

hey @rafaelab,

when I see it correctly the CandidateTagColumn is not default in all OutputTypes. Those Types designed for the trajectory information (Trajectory1D and Trajectory3D) don't have the column. I decided to add these only to the Event1D/3D types as these are meant to analyze interaction processes, where the tag information is helpful.

I can understand the problems with changing the default output behavior, as it will break the existing analysis scripts. This problem will always occur when using TextOutput("filename.txt") as in this case the enableAll() function is called which activates also the CandidateTagColumn. As the name suggest "all features", I would prefer to leave it this way. From my point we can discuss to leave out the tag in the Event types but this will not solve all problems and I think that all users can understand that migrating to a newer version can lead to adaptions in the code.

rafaelab commented 1 year ago

Hi @JulienDoerner,

Thanks for the reply. Personally, I don't mind changing default behaviours and I think that should not be a constraint for developers. In fact, I would change things much more often than what we do. But for this particular case I don't see any particular gain.

Why would users need to have it by default in EventND? Sure, it helps in some analyses, but quite often the people are interested in just plotting all particles of a given type. Thinking of UHECRs and gamma rays, I can hardly think of situation that justifies making outputs ~15% larger by default, without any specific gain in mind. I think it makes sense to have the tags by default in TrajectoryOutput, since it tells you exactly what happened at each step to generate a given particle and it breaks degeneracy in a case where the user is explicitly studying what happens at each step. This is not true for EventND.

It is great to have the option to add the tags, and I can certainly see the need to have it as a candidate property, even if it slows down CRPropa a bit, but this information is not generally needed. That is why I am suggesting this to be, by default, disabled for this type of output, and whoever requires this information can simply enable it.