nu-radio / NuRadioMC

A Monte Carlo simulation package for radio neutrino detectors
GNU General Public License v3.0
33 stars 35 forks source link

Refactoring coreas to include interpolation #688

Open cg-laser opened 4 months ago

cg-laser commented 4 months ago

Major refactoring of the coreas interface to read the electric field from coreas hdf5 files. A new class readCoREASDetector.py can be used to read an array and get the interpolated position.

This is largely a copy of #685 which is an undefined state because of an accidental force push. Please never force-push unless you know what you are doing.

cg-laser commented 4 months ago

@MijnheerD @lpyras I'm getting these warning from the interpolator module all the time:

Trace arrival times were not set during init, only relative timings are returned!
warning: negative values in abs_spectrum found: 6 times. Setting to zero.

are we not using the interpolator correctly?

cg-laser commented 3 months ago

@MijnheerD @lpyras I added an example of a "LOFAR"-style Xmax reco. The example is using a vertical shower. It would probably be good to also test with an inclined shower as some potential bugs might only appear then.

Before merging this PR, it would be good if the module would be tested a bit more. Is my understanding correct, that this module has not been used by anyone and is essentially untested?

cg-laser commented 3 months ago

The output of the reconstruction looks like this. A remaining ToDo is a correct uncertainty estimation of the energy fluence. This should be implemented into the efieldSignalReconstructor

Xmax_fit_0 footprint_0_core0 000000_0 000000

lpyras commented 1 month ago

@MijnheerD @lpyras I'm getting these warning from the interpolator module all the time:

Trace arrival times were not set during init, only relative timings are returned!
warning: negative values in abs_spectrum found: 6 times. Setting to zero.

are we not using the interpolator correctly?

@MijnheerD I dont really know what that means. I always thought we are anyway only interested in the relative timing. I implemented access now the timing information provided by the cr-pulse-interpolator. Is there something else to do?

lpyras commented 1 month ago

@MijnheerD @lpyras I added an example of a "LOFAR"-style Xmax reco. The example is using a vertical shower. It would probably be good to also test with an inclined shower as some potential bugs might only appear then.

Before merging this PR, it would be good if the module would be tested a bit more. Is my understanding correct, that this module has not been used by anyone and is essentially untested?

I tested it before the coreas refactoring for effective area calculation. That yielded reasonable results.

lpyras commented 1 month ago

I changed everything we discussed so far e.g. change the coreas readers in a way that they only rely on the readCorsika7() function. Unfortunately I didn't have time to test everything. Any volunteers? Otherwise I try to do it in the next weeks. :)

MijnheerD commented 1 month ago

Sorry for coming in late on this, I thought I already answered some the questions but apparently I didn't.

@MijnheerD @lpyras I'm getting these warning from the interpolator module all the time:

Trace arrival times were not set during init, only relative timings are returned!
warning: negative values in abs_spectrum found: 6 times. Setting to zero.

are we not using the interpolator correctly?

@MijnheerD I dont really know what that means. I always thought we are anyway only interested in the relative timing. I implemented access now the timing information provided by the cr-pulse-interpolator. Is there something else to do?

This is important, because usually the relative timing is not enough. The problem is that for larger starshapes, the CoREAS time traces start at different times. We added the option to pass the start times to the interpolator, which it can then interpolate for you as well. The warning tells you that the start times were not provided to the constructor. On the lofar/interpolator branch we added this on line 65 of "readCoREASInterpolator.py". Then on line 191 we retrieve the start times, which are used in the make_sim_station function give the traces the correct start time.

@MijnheerD @lpyras I added an example of a "LOFAR"-style Xmax reco. The example is using a vertical shower. It would probably be good to also test with an inclined shower as some potential bugs might only appear then.

Before merging this PR, it would be good if the module would be tested a bit more. Is my understanding correct, that this module has not been used by anyone and is essentially untested?

I agree we should test it more, and I think once the LOFAR pipeline is done we will have an excellent playground for that. Although we can also already start testing on simulations, like the Xmax example (thanks @cg-laser for the script!). I am not sure when we will be able to get to it, but in September we should have a master student who will need to work with the interpolator so he might be able to test it more thoroughly.

anelles commented 1 month ago

I just tested the branch and it actually fails with a pip installed cr_interpolator on: File "/usr/local/lib/python3.12/site-packages/cr_pulse_interpolator/signal_interpolation_fourier.py", line 9, in import interpolation_fourier as interpF ModuleNotFoundError: No module named 'interpolation_fourier I feel like I have seen this error discussed. Do we need a new cr_interpolator release?

anelles commented 1 month ago

So the automatic build fails on the cr_interpolator not being installed, but even if you pip install it, it doesn't work.

anelles commented 1 month ago

Ah, @lpyras just reminded me that this is known and a new release is being worked on. Then I can do the homework as assigned by @MijnheerD to me check whether the reconstruction is indeed what we have done for LOFAR.

lpyras commented 1 month ago

I try to address @MijnheerD suggestions.

One question still open is the z coordinate of the shower. What should we do in sim_station? The height of the coreas shower? Where do we want the core position? In the same plane as the antenna?

Should we remove make_sim_station() and make_sim_shower() as Mitja suggested?

Any opinions? @cg-laser @fschlueter