nu-radio / NuRadioReco

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

RNO_G hardware response #202

Closed lpyras closed 4 years ago

lpyras commented 4 years ago

Issue being addressed: This pull request includes a surface detector discription for RNO-G, the hardware response for the amplifier IGLU and rno-surface. It is not yet connected to the database.

cg-laser commented 4 years ago

Hi Lilly, thanks for working on that. I haven't looked at the changes but just a general comment: The initial message is supposed to contain a description of the pull request. You should replace the template starting with "issues being addressed" etc. This is just a reminder to do certain things before you open the pull request. Could you add a description of the pull request below? Thanks

lpyras commented 4 years ago

Hi Lilly, thanks for working on that. I haven't looked at the changes but just a general comment: The initial message is supposed to contain a description of the pull request. You should replace the template starting with "issues being addressed" etc. This is just a reminder to do certain things before you open the pull request. Could you add a description of the pull request below? Thanks

Sure, I didnt noticed that window. I added it now.

anelles commented 4 years ago

The tests show differences in the high-low and in the multi-high low trigger. Did you inadvertedly change something about the old triggers? I didn't see it tough ...

anelles commented 4 years ago

Please make a dummy commit to trigger the built-bots again.

lpyras commented 4 years ago

The tests show differences in the high-low and in the multi-high low trigger. Did you inadvertedly change something about the old triggers? I didn't see it tough ...

The only thing I did not do intentionally was this change in the base_station. But it seems okay to me.

def remove_triggers(self): self._triggers = collections.OrderedDict()

cg-laser commented 4 years ago

I think the tests were failing because of a recent change in NuRadioMC. The NuRadioReco master was updated accordingly, but your branch didn't contain these changes yet. I just merged the master branch back into this branch. It should work now. Make sure that you do a git pull before you do any new commit to avoid merge conflicts.

cg-laser commented 4 years ago

I saw that you added several measurement files. How big are they? I think we should try to avoid adding to many measurement files to the repository. We have a different mechanism of providing larger files. If this is only considered an example for one demo RNO station it is probably fine

lpyras commented 4 years ago

I saw that you added several measurement files. How big are they? I think we should try to avoid adding to many measurement files to the repository. We have a different mechanism of providing larger files. If this is only considered an example for one demo RNO station it is probably fine

They are needed for the hardware response. i removed a few redundant files. Now, all files together have a volume of ~2,4 MB.

christophwelling commented 4 years ago

I just noticed that the RNO analog_components.py returns the phase as an angle while the others return it as a complex number. Could you change that to make it work the same way for all detector types?

lpyras commented 4 years ago

I just noticed that the RNO analog_components.py returns the phase as an angle while the others return it as a complex number. Could you change that to make it work the same way for all detector types?

To me, it looks like there is no unique way. ARA use the angle if the measurement is loaded while the complex number is used in the get_ function. In ARIANNA load_amplifier_response uses the complex number and in load_amp_measurement they just use the angle. I think the way I implemented it here should be okay. But I can change it to complex numbers in the file and the HardwareResponseIncorporator.

christophwelling commented 4 years ago

But the function that is used to get the response for given frequencies always returns it as a complex number. Just look at how they are used in the hardwareResponseIncorporators