nu-radio / NuRadioReco

reconstruction framework for radio detectors of high-energy neutrinos
GNU General Public License v3.0
5 stars 3 forks source link

Return copy of trace and spectrum #279

Closed christophwelling closed 3 years ago

christophwelling commented 3 years ago

The get_trace and get_frequency_spectrum methods return a reference to self._time_trace or self._frequency_spectrum. This is dangerous, because modifying the returned traces can change the values stored by the trace object to change in unintuitive ways: ` In [1]: import numpy as np

In [2]: import NuRadioReco.modules.io.NuRadioRecoio

In [3]: io = NuRadioReco.modules.io.NuRadioRecoio.NuRadioRecoio(['output.nur'])

In [4]: channel = io.get_event_i(0).get_station(101).get_channel(0)

In [5]: np.max(channel.get_trace())
Out[5]: 9.21563751242359e-05

In [6]: spec = channel.get_frequency_spectrum()

In [7]: np.max(channel.get_trace())
Out[7]: 9.215637512423589e-05

In [8]: spec *= 1.e6

In [9]: np.max(channel.get_trace())
Out[9]: 9.215637512423589e-05

In [10]: spec = channel.get_frequency_spectrum()

In [11]: spec *= 1.e6

In [12]: np.max(channel.get_trace())
Out[12]: 92.15637512423588 ` Weather or not modifications to spec affect the trace stored in the channel depends on whether it is stored in the time or frequency domain. That is why the trace maximum changes in In[12] but not in In[9]. By returning a copy of the trace we can stop this from happening.

cg-laser commented 3 years ago

I'm unsure if this is a good idea. The current behavior is just how python works. But I agree it is confusing/obscure. I'm worried that this change will impact performance and memory consumption. Do we have a way to evaluate that?

christophwelling commented 3 years ago

We could check if the tests take any longer with this branch. The thing I am worried about is someone accidentally changing traces without noticing it, for when multiplying it with units for plotting. That can be very easy to miss, especially as it only happens under specific circumstances that are not at all obvious unless you know what the trace class looks like on the inside. I found this issue the hard way, and it took me quite some time to figure out what was going on.