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

Bug in coordinatesystems.py #4

Closed fschlueter closed 5 years ago

fschlueter commented 5 years ago

When running: the test code in coordinatesystems.py (python2 radiotools/coordinatesystems.py)

File "radiotools/coordinatesystems.py", line 138, in transform_to_vxB_vxvxB nX, nY = station_position.shape ValueError: need more than 1 value to unpack This happens when transforming a single station position. Line 143 looks like it should catch this case (array with shape (3,)) however it never reaches this point. if(len(station_position.shape) == 1): Adding somethink like # if a single station position is transformed: (3,) -> (1, 3) if station_position.ndim == 1: station_position = np.expand_dims(station_position, axis=0) would solve this issue. However than the test code does not return same values for the vector is both coordinate systems (are they supposed to do that?).

fschlueter commented 5 years ago

Same for function transform_from_vxB_vxvxB

cg-laser commented 5 years ago

the 'test' code was pretty outdated, so I removed it. It was anyway not a good idea to have test code in the same file, all test should go into a separat file.

Your suggestion looks like it would work, but I think this case is not needed anywhere at the moment. So I would implement a change only if it is necessary.

fschlueter commented 5 years ago

Hi, yes separated unit test would be really nice :).

I think this functionality is needed. One case you would like to transform a single station position is when creating a list of station positions for, e.g., simulations. Often it is easier to perform a simple for loop and process the positions individually (even though that might not be really pythonic). And I think it is actually used in coreas.write_list_star_pattern (or am I fooling myself?). Moreover in the function "description" it is says that "transform a single station position or a list of multiple station positions back to x,y,z CS"

cg-laser commented 5 years ago

ok convinced. We just need to be sure that the current behavior is not effected by your changes. If you have time, it would be great if you can set up some unit tests. Just transform a bunch of positions into the vvB frame and compare it with the expected solution. Or/and transform back and fourth and check it positions remain the same.

fschlueter commented 5 years ago

With merging of branch #5 #6 #8 and #9 we can close this issue :)