-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NuRadioMC 3.0 #680
base: develop
Are you sure you want to change the base?
NuRadioMC 3.0 #680
Conversation
In order to use the trace multipolication for the birefringence calculations the branches were merched.
Merge branch 'develop' of https://github.com/nu-radio/NuRadioMC into birefringence
@cg-laser it looks like the diff includes the changes of the birefringent2 branch which was merged. Can you try to remove it from the PR? Maybe by either merging or rebasing the current development branch in this one? |
for iCh in det.get_channel_ids(sim_station_id): | ||
if channel_ids is None: | ||
channel_ids = det.get_channel_ids(sim_station_id) | ||
for iCh in channel_ids: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could use the same variable as in the previous loop (channel_id
)
|
||
Parameters | ||
---------- | ||
evt : Event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sjoerd can comment better, but we could use references to the class object here which is reflected in the deployed documentation
One more question with regards to the new looping structure - how would you treat e.g. coherent noise sources (like the galaxy)? This is currently still WIP (see PR #591), but eventually we will probably want to have galactic noise that is coherent between different antennas (I'm not sure about deep detectors, but certainly for e.g. LOFAR this will be useful for calibration). How would that fit into the new looping structure? |
# initialize a few NuRadioReco modules | ||
# TODO: Is this the best way to do it? Better to initialize them on demand | ||
channelAddCableDelay = NuRadioReco.modules.channelAddCableDelay.channelAddCableDelay() | ||
electricFieldResampler = NuRadioReco.modules.electricFieldResampler.electricFieldResampler() | ||
efieldToVoltageConverterPerEfield = NuRadioReco.modules.efieldToVoltageConverterPerEfield.efieldToVoltageConverterPerEfield() | ||
efieldToVoltageConverter = NuRadioReco.modules.efieldToVoltageConverter.efieldToVoltageConverter() | ||
channelSignalReconstructor = NuRadioReco.modules.channelSignalReconstructor.channelSignalReconstructor() | ||
channelResampler = NuRadioReco.modules.channelResampler.channelResampler() | ||
channelGenericNoiseAdder = NuRadioReco.modules.channelGenericNoiseAdder.channelGenericNoiseAdder() | ||
channelResampler = NuRadioReco.modules.channelResampler.channelResampler() | ||
eventWriter = NuRadioReco.modules.io.eventWriter.eventWriter() | ||
triggerTimeAdjuster = NuRadioReco.modules.triggerTimeAdjuster.triggerTimeAdjuster() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I am not a fan of calling the object the same as the module/class. If we would change that we could also simplify the module imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is your suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same names but in snake_case? channelAddCableDelay
-> channel_add_cable_delay
or something similar like channel_cable_delay_adder
detector_simulation_filter_amp: function (optional) | ||
a function that applies the filter and amplifier response to the electric field | ||
the arguments to the function are (event, station, detector) | ||
if not provided, the function `detector_simulation_part1` needs to be provided. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here and in the following it should be _part2
and not _part1
. Also these names are a bit confusing. Can we find better names?
trace = np.zeros(n_samples) | ||
trace[n_samples // 2] = 100 * units.V # set a signal that should satisfy any trigger and speedup cuts | ||
trace[n_samples // 2 + 1] = -100 * units.V | ||
electric_field.set_trace(np.array([np.zeros(n_samples), trace, trace]), 1. / dt) | ||
electric_field.set_trace_start_time(0) | ||
electric_field[efp.azimuth] = 0 | ||
electric_field[efp.zenith] = 100 * units.deg | ||
electric_field[efp.ray_path_type] = 0 | ||
sim_station.add_electric_field(electric_field) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that necessary?
One general comment / question. I appreciated that Christoph's solution spread the code over several files. In my opinion that increases readability (of course there can be a to much files), instead of having one file with +1500 lines. |
os.path.basename(path))) | ||
download_file = True | ||
else: | ||
logger.warning("no hash sum of {} available, skipping up-to-date check".format(os.path.basename(path))) | ||
logger.status("no hash sum of {} available, skipping up-to-date check".format(os.path.basename(path))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is actually worth a warning?
efield_ids = [] | ||
for efield in self._electric_fields: | ||
efield_ids.append(efield.get_unique_identifier()) | ||
efield_ids.sort() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not necessarily expect this sort here. Maybe add it to the information to the doc string
@@ -322,3 +332,19 @@ def deserialize(self, data_pkl): | |||
self.set_station_time(data['_station_time']) | |||
|
|||
self._particle_type = data['_particle_type'] | |||
|
|||
|
|||
def __add__(self, x): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc string missing. Is it intuitive to implement such a operator? Maybe we should implement a merge
or combine
function
""" | ||
Get an iterator over all simulated showers in the event | ||
""" | ||
for shower in self.__sim_showers.values(): | ||
yield shower | ||
return self.__sim_showers.values() | ||
# for shower in self.__sim_showers.values(): | ||
# yield shower |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change the doc-string. Typically this kind of functions returns an iterator right? Why changing that for this one?
if det is None: | ||
logger.status("Detectorfile {}".format(os.path.abspath(self._detectorfile))) | ||
kwargs = dict(json_filename=self._detectorfile, default_station=default_detector_station, | ||
logger.status("Detectorfile {}".format(os.path.abspath(detectorfile))) | ||
kwargs = dict(json_filename=detectorfile, default_station=default_detector_station, | ||
default_channel=default_detector_channel, antenna_by_depth=False) | ||
kwargs.update(det_kwargs) | ||
self._det = detector.Detector(**kwargs) | ||
else: | ||
self._det = det |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we break backwards compatibility? If so I am opting for removing the initialisation of the detector object from this class. The object should be passed to the class and initialized in the top level script. This also forces the user to think about it a bit more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also finally remove the use of the deprecated default_channel/station
Good question. This is important to consider. I think I would implement an internal buffer of the genericNoiseAdder. As long as the |
This PR is a complete refactor of the NuRadioMC core (simulation.py), enabling long-awaited features such as:
The key was to change the internal looping structure from
![image](https://private-user-images.githubusercontent.com/31775679/332448835-421df7ca-d045-4ff6-bee0-e2c56cfab566.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTg3MzAwODMsIm5iZiI6MTcxODcyOTc4MywicGF0aCI6Ii8zMTc3NTY3OS8zMzI0NDg4MzUtNDIxZGY3Y2EtZDA0NS00ZmY2LWJlZTAtZTJjNTZjZmFiNTY2LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA2MTglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNjE4VDE2NTYyM1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTg0NWYwYTI0ZjZmNjkzY2Y1ZmY0YjlmOTlhZjBjNmQ2ZGNjZjQ3YjU3MDUxMTczNTRlZDI3NTcwMjdiMzMxODMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.2cM-wpej0GwEoOCsX4HgQXM_vqC00UxILnIXnmM_gfA)
to
Changes will be discussed in upcoming InIceMC calls. More information will be provided to the PR later.