nu-radio / NuRadioReco

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

Issues with singleton class for detectors #288

Closed clark2668 closed 3 years ago

clark2668 commented 3 years ago

Describe the issue The singleton class create_new method doesn't work as intended. For example, in the following, the stations in the "deep detector" seems to inherit the locations of the shallow detector--even though I've passed create_new=True.

det_deep = detector.Detector(json_filename='pa_200m_2.00km.json', antenna_by_depth=False, create_new=True)
det_deep.update(Time.now())
deep_station_ids = det_deep.get_station_ids()

det_shallow = detector.Detector(json_filename='surface_4LPDA_PA_15m_RNOG_300K_1.00km.json', antenna_by_depth=False, create_new=True)

for d_id in deep_station_ids:
    loc_deep = det_deep.get_absolute_position(d_id)
    print("Station {}, Deep {}".format(d_id, loc_deep)) # should be the position of deep station, but it reports the shallow locations

The det files are attached: det_files.zip

It is the case that the class instances are different:

print(det_deep)
print(det_shallow)
print(det_deep)
<NuRadioReco.detector.detector.Detector object at 0x7fcb80f6b790>
<NuRadioReco.detector.detector.Detector object at 0x7fcb87a69ac0>
<NuRadioReco.detector.detector.Detector object at 0x7fcb80f6b790>

so there must be something subtle in the class internal working going wrong maybe?

Expected behavior The singleton class should generate a new class instance, but doesn't seem to be doing so.

Additional context Thought this had been fixed in #266 , but or maybe I'm just doing something wrong.

christophwelling commented 3 years ago

I couldn't recreate the problem. I create the shallow and deep detector instances from your JSON files, and both seemed to return the correct positions. I tried it for a few stations IDs. Can you post the full script and the console outputs? For example, trying to access det_shallow with this script should throw an error that the detector time is not set. You probably want to use the GenericDetector class anyway, so that all the stations have channels.

clark2668 commented 3 years ago

Hi, sorry for the delay here. Yes, good idea, let me attach the full files and script.

demo.zip

What I'm attaching in the zip file is the script (go.py), and the results of running with and without L8 commented out. When L8 is left in, it prints the following:

Station 1, Deep [-10000. -12500.      0.]
Station 2, Deep [-10000. -11500.      0.]
Station 3, Deep [-10000. -10500.      0.]
Station 4, Deep [-10000.  -9500.      0.]
Station 5, Deep [-10000.  -8500.      0.]
...

Which you can see has a 1km vertical spacing, which is wrong. With L8 commented out, I get the following:

Station 1, Deep [-10000. -12500.      0.]
Station 2, Deep [-10000. -10500.      0.]
Station 3, Deep [-10000.  -8500.      0.]
Station 4, Deep [-10000.  -6500.      0.]
Station 5, Deep [-10000.  -4500.      0.]
...

Which has the expected 2km vertical spacing. I actually specifically did not try to use det_shallow just to confirm that the problem existed just by invoking a new instance. In case it helps, I am running NuRadioMC on the improve-limit branch (commit f28220e) and am on the NuRadioReco master (commit f865f1b1).

Thanks for all the help!

christophwelling commented 3 years ago

Thanks for the files, that helped a lot. I created a pull request that should fix this: https://github.com/nu-radio/NuRadioReco/pull/294 Can you test if everything works now?

christophwelling commented 3 years ago

Fixed in #294