nu-radio / NuRadioReco

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

Singleton requirement in detector makes it difficult to work with >1 detector files #266

Closed clark2668 closed 3 years ago

clark2668 commented 3 years ago

Describe the issue Because the detector class is declared as a singleton, it is impossible to load two detector files at once, which might be useful for standalone purposes. For example, you can't do something like this:

dets = []
list_of_files = ['file1.json', 'file2.json']
for file in list_of_files:
  det = detector.Detector(json_filename=file, antenna_by_depth=False)
  dets.append(det)

You end up getting two copies of the first file:

print(dets)
[<NuRadioReco.detector.detector.Detector object at 0x7fa818050c40>, <NuRadioReco.detector.detector.Detector object at 0x7fa818050c40>]

Which is not helpful, and actually kinda confusing as to why the behavior is like that

Expected behavior You'd prefer to be allowed to instantiate an indefinite number of detector objects.

Additional context N/A

cg-laser commented 3 years ago

@christophwelling I just added you to this issue because I think to remember that you ran into a similar problem?

The general problem is that we want both options. In a simulation, we don't want to create a new detector instance all the time, therefore the singelton. But for other cases it might be benefitial to open several detectors at once.

christophwelling commented 3 years ago

You can create a new detector object by calling the __new__ method explicitly: detector = NuRadioReco.detector.detector.Detector.__new__(NuRadioReco.detector.detector.Detector) detector.__init__(source='detector_file.json') This is for example done in NuRadioRecoio, when we read a detector from a .nur file and create a new detector object for each event file.

clark2668 commented 3 years ago

Ahhhh, I see. Is that obvious? If so, then chalk it up to my ignorance and we can move on :) If not, can we put a note about this somewhere in the docs?

cg-laser commented 3 years ago

I agree that is not obvious. I would not have know it myself. @christophwelling would it be possible to just add a helper function like initialize_new_detector to the detector.py file which then executes exactly the code you posted above?

christophwelling commented 3 years ago

Yes, I agree it is not really obvious, I also had to google it at the time. It's one of those things that are built in python, but you basically never have a reason to use. I created this PR: https://github.com/nu-radio/NuRadioReco/pull/267 to fix this. Now you can just pass a parameter to the creater to get a new instance

anelles commented 3 years ago

This looks like the issue was fixed in #267 @christophwelling, right? If you confirm, can you close this issue?

christophwelling commented 3 years ago

Yes, this can be closed.