search

nu-radio / radiotools

A tool package for cosmic-ray and neutrino radio detectors
GNU General Public License v3.0
8 stars 5 forks source link

Fix coordinatesystems #6

Closed fschlueter closed 5 years ago

fschlueter commented 5 years ago

This branch contains the commit of the unittest branch. Its fixes the issue in the coordinatesystems.py (2. commit, issue #4). With the 3. commit temporary code in the unittests (to "bypass" issue #4) is replaced.

Tests passing

cg-laser commented 5 years ago

I'm not sure if your modified code work properly. Keep in mind that we have two use cases:

This function is supposed to transform time traces with the shape (number of polarizations, length of trace) and a list of station positions with the shape of (length of list, 3). The function automatically differentiates between the two cases by checking the length of the second dimension. If this dimension is '3', a list of station positions is assumed to be the input. Note: this logic will fail if a trace will have a shape of (3, 3), which is however unlikely to happen.

Can you add these two use cases into your unit test?

You also removed the deep copy of station positions. This was neccessary because loop with 

for pos in station_positions:
    pos -= core

will alter the station_positions array. In python complex types (such as lists or arrays) are passed by reference.

fschlueter commented 5 years ago

Hi, you are right, I was just thinking that station_position is not used any further in the function so that changes of this result will not affect what the function does. However it changes the variable (also for the "outside") which is properly not desired. I will add an appropriate line.

Regarding your comment on the use cases: i did not change the behavior how to differentiate between the two cases and the case transforming traces remains untouched (as far as i can see). I removed the logic to catch len(station_positions.shape) == 1 as this can not be reached anymore (would have cased the error in the first place).

fschlueter commented 5 years ago

Hi @cg-laser do you want to have a look at the newest changes? I added appropriate test for all cases (and they are passing). The transformation of the traces is checked with "true" values generate with the CoREAS trunk. I also did similar changes to transform_from_vxB_vxvxB_2D however there is not a test for this function yet. Since I am not pretty sure if i understand the function: Transformation and translation of a point in shower coordinates to a corresponding point in xyz coordinate on the ground plane), I would like to ask if you could add an appropriate test and merge this.