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

Vector normalization of getRotataed() in Vector3.h #393

Closed simonerossoni1996 closed 2 years ago

simonerossoni1996 commented 2 years ago

Dear all,

I have fixed a bug present in the function getRotated() of Vector3.h. All the details and tests can be found in the presentation

ConeEmission_Targeting_issues.pdf

I tried to included this modification in the function randVectorAroundMean() of Random.cpp instead of Vector3.h. I ran a speed test to check the impact of these two different modifications on code performance. Here the result where Method 1 corresponds to the change in Vector3.h and Method 2 corresponds to the change in Random.cpp.

speed-test.pdf

For a large number of simulated particles the two method do not differ greatly. Therefore I suggest to modify the function getRotated() in Vector3.h in order to make this function mathematically correct and usable without further modifications.

Thank you.

rafaelab commented 2 years ago

Hi @simonerossoni1996

The PR looks fine to me. To keep a uniform code style, please follow the conventions from: https://github.com/CRPropa/CRPropa3/blob/master/CONTRIBUTING.md This essentially means: add white spaces around the division: Vector3<T> u = axis / axis.getR();

One more thing: make sure that testVector.cpp is still passing after this change.

simonerossoni1996 commented 2 years ago

Hi @rafaelab

thanks for your comments. I modified the style so that it is uniform now. I included two more lines in the code since there was a problem with the test rotation around the zero axis. Now all the tests are passed.

Thank you again.

JulienDoerner commented 2 years ago

Hi @simonerossoni1996,

the PR looks fine for me. I have only one small comment: Can you add a short sentence to the Changelog https://github.com/CRPropa/CRPropa3/blob/master/CHANGELOG.md. If it is done the PR can be merged

simonerossoni1996 commented 2 years ago

HI @JulienDoerner ,

thanks for your comment. I added a new line in the Changelog file.