Update phased trigger and digitizer #799
Conversation
…phased_trigger_and_digitizer
fschlueter
changed the base branch from
develop
to
simulate_trigger_channels_rnog_simplified
|
@ryankrebs016 I merged the branch |
| @@ -0,0 +1,349 @@ | |||
| #!/bin/env python3 | |||
There was a problem hiding this comment.
what is the purpose of this script? Compared to simulate.py does it only add the two triggers and allows to use the NuRadioMCRunner? Can we not merge the two scripts into one (maybe we can have an second script which takes the simulation as defined in the first script but uses the runners?)
| Vrms = fin.attrs['Vrms'] | ||
| try: | ||
| Vrms = fin.attrs['Vrms'] | ||
| except: | ||
| Vrms = 1 |
There was a problem hiding this comment.
hm, why is Vrms not available? It should? Is 1 a good default value? I think you might be able to make this code a one-liner Vrms = fin.attrs.get('Vrms', 1) if hdf5 attributes are compatible with python dictionaries.
| "trigger_adc_noise_nbits": 3.321, | ||
| "trigger_adc_noise_nbits": 2.585, |
There was a problem hiding this comment.
has there been a change in the firmware? Is that available in the data?
There was a problem hiding this comment.
No changes to firmware. See the next two comment threads for why this number changed
There was a problem hiding this comment.
Okay but you removed the -1 there, why would you change this number here? Or do you only have to change the nbits if the -1 is in the voltage/adc conversion? Sorry something does not yet add up for me.
|
|
||
| lsb_voltage = adc_ref_voltage / (2 ** (adc_n_bits - 1) - 1) | ||
|
|
||
| lsb_voltage = adc_ref_voltage / (2 ** (adc_n_bits) - 1) |
There was a problem hiding this comment.
See discussion in #500, if we change this (which I in principle would be very happy with) we would need to update all existing json detector files and would break the code for all users with their own detector files in a very subtle and dangerous way.
There was a problem hiding this comment.
This just makes the code kind of confusing to follow. To me I would use lsb_voltage = voltage_range / (2^adc_n_bits -1). Instead, a ref_voltage is used and is calculated using the code in the comment below these.
There was a problem hiding this comment.
I absolutely agree with you, I stumbled across this twice (more than a year apart from each other so I had forgotten). The issue is if we change it now we will effect all other users using this module. Maybe we should have this discussion in the issue I linked above?
| adc_ref_voltage = Vrms * (2 ** (adc_n_bits - 1) - 1) / (2 ** (adc_noise_n_bits - 1) - 1) | ||
| adc_ref_voltage = Vrms * (2 ** (adc_n_bits) - 1) / (2 ** (adc_noise_n_bits ) - 1) |
|
I like the idea of having a base class for the phased array trigger very much! But I think we should think about restructuring the code a bit. First, can you rename the phased array trigger modules to reflect the typically name scheme for modules in NuRadio? Should we put all modules in |
Changes: