Closed JulienDoerner closed 2 years ago
Hi @JulienDoerner
As a general comment about style, please take a look at our contributing guidelines.
Except for these very minor style issues, this PR seems fine to me. I cannot perform further tests regarding the physics, but I trust you did it yourself.
Hey @rafaelab, I have also seen thath the code is not in the typical CRPropa Style. But as it was styled so before, I tried to change as little as possible to get backward compatibility. If you think it is more importend to get a continues code styling I could check it agnainst and change the other functions as well.
Hi @JulienDoerner I personally have very strong opinions about coding conventions. In this case, I do see the point for the underscore in the setters and getters and I'm open for discussing it (although I would rather not have them). But I don't see why the rest of the code should not conform with the style (e.g., whitespaces, etc). I had a PR some time ago fixing all these small style-related things. Many parts of CRPropa come from other codes. The question is how difficult it would be to make a piece of code CRPropa-like. Your PR is just one small functionality that could even be external to CRPropa, so in adding it to the repository, I don't see why not to follow the guidelines. Also, it is quite small, so not a lot of work :) This is not the same situation as in the PhotoPionProduction module. There it makes sense to preserve SOPHIA's function names in the C++ to be consistent with the Fortran code and make our lives easier.
Thanks for your explanation @rafaelab, I personally agree with your opinion about coding conventions but in previous discussion I thought that backward compatiblity was higher rated than coding style. However, for this PR I will apply the conventions for the other parts of the code to, as it will not be much work and only small changes for the users.
Hi @JulienDoerner I just realised now that this PR does much more than is described in your first comment. Is the introduction of the new DiffusionTensor interface mandatory for the SNR distribution? I do not see that connection. In case there is really no connection I strongly suggest to split this PR into two. Otherwise it will be harder to disentengle thing in the future. Sorry that I noticed it so late.
@lukasmerten, sorry. That was not intended. I have merged the wrong branch. I try to undo it.
@JulienDoerner The current list of changed files looks ok now. The problem is still within the git history, though. There the commits of the DiffusionTensor etc. still show. The easiest solution might be to create a new clean PR based on the current master. Then we could close this PR here and merge the new one. What do you think?
@lukasmerten, I agree with you. Creating a new PR will be the easiest soulution.
Dear all,
Here I introduce a new model parameter (called α) for the radial distribution of SNRs in the MiklyWay (see https://doi.org/10.1093/mnras/stv1885) for the model.
best regards, Julien