galactic noise adder upgrades #740
Conversation
more sky models, speed up module
| except: | ||
| logger.warning("radiocalibrationtoolkit import failed. Consider installing it to use more sky models.") | ||
|
|
||
| try: | ||
| from pylfmap import LFmap # Documentation: https://github.com/F-Tomas/pylfmap needs cfitsio installation | ||
| except: | ||
| logger.warning("LFmap import failed. Consider installing it to use LFmap as sky model.") |
There was a problem hiding this comment.
Please replace these with except ImportError
| self.__interpolation_frequencies = np.around( | ||
| np.logspace( | ||
| *np.log10(freq_range / units.MHz), num=30 | ||
| ), 0 | ||
| ) * units.MHz |
There was a problem hiding this comment.
I'm guessing (by comparison to the number of interpolation frequencies previously) 30 points is usually plenty - we can always add this as an additional optional argument if that seems useful.
Also, nitpicky - dividing and then multiplying by units.MHz is probably superfluous.
| last_freqs = None | ||
| for channel in station.iter_channels(): | ||
| if (not last_freqs is None) and ( | ||
| not np.allclose(last_freqs, channel.get_frequencies(), rtol=0, atol=0.1 * units.MHz)): | ||
| logger.error("The frequencies of each channel must be the same, but they are not!") | ||
| return | ||
|
|
||
| last_freqs = channel.get_frequencies() |
There was a problem hiding this comment.
It's kind of 'implicit' in NuRadioMC that all channels in a station have the same sampling rate. Is there a specific use case you are aware of that would trigger this?
Anyway, it's always good to have additional checks, so I don't think this needs to be removed.
There was a problem hiding this comment.
I think these were also changes that Moritz implemented - I am also not sure in which case they would be needed, but he clearly deemed this important. I also think we can keep it since it does not seem to cause any problems
| interpolation_frequencies: array of float | ||
| freq_range: array of len=2, default: [30, 80] * units.MHZ | ||
| The sky brightness temperature will be evaluated for the frequencies | ||
| in this list. Brightness temperature for frequencies in between are | ||
| within this limit. Brightness temperature for frequencies in between are | ||
| calculated by interpolation the log10 of the temperature | ||
| The interpolation_frequencies have to cover the entire passband | ||
| specified in the run method. |
There was a problem hiding this comment.
Changing signatures of existing functions without a deprecation warning is not ideal. Maybe we should keep interpolation_frequencies and add the new freq_range (or maybe min_freq and max_freq?) as additional optional arguments, but raise a DeprecationWarning every time someone uses interpolation_frequencies. We can then fully deprecate in a future release.
(This is another 'coding practice' thing that we have mostly ignored in the past, and I don't think it's particularly dangerous in this case, but maybe it's something we should start doing)
There was a problem hiding this comment.
...alternatively, we can keep the argument interpolation_frequencies. Your call.
There was a problem hiding this comment.
Good suggestion, I kept both and print a warning when interpolation_frequencies is used.
I much prefer freq_range since it does not require the user to set up the frequencies themselves, especially since log interpolation seems like a more ideal choice for Galactic Noise.
| if freq_range is None: | ||
| freq_range = np.array([30, 80]) * units.MHz |
There was a problem hiding this comment.
Relatedly - changing from a default of [10,1100] to [30,80] MHz is quite a big change, and e.g. for RNO-G the new default is definitely not what we want. I would prefer to keep the old default and just raise an error (or a warning) if the chosen model is not compatible with the frequency range.
There was a problem hiding this comment.
(oops, I changed the default for LOFAR related reasons but meant to change it back for the commit)
| delta_phases = ( | ||
| 2 * np.pi * freqs[passband_filter] / (scipy.constants.c * units.m / units.s) * n_air * | ||
| ( | ||
| sin_zenith * | ||
| (np.cos(azimuth) * channel_pos_x + channel_pos_y * np.sin(azimuth)) + | ||
| channel_depth * | ||
| ((n_ice / n_air) ** 2 + sin_zenith ** 2) / | ||
| np.sqrt((n_ice / n_air) ** 2 - sin_zenith ** 2) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I think this is Moritz' change to account for the same signal in different antennas. This is definitely an upgrade we want to have, but as mentioned in #591 I don't think this is correct for deep in-ice antennas (I am okay with not fixing that for now, although maybe issuing a warning for channel depths over 10 m would be good?), and it also doesn't look right to me for above-surface antennas right now (that probably should be fixed and should be relatively straightforward).
| # add random polarizations and phase to electric field | ||
| polarizations = np.random.uniform(0, 2. * np.pi, len(S)) |
There was a problem hiding this comment.
...also an open question whether random polarizations are right (technically, this implies that the signal from each signal is fully linearly polarized in a random direction for each antenna. I don't think this is right, we should think about how to do this correctly, even if the impact in the end is probably small).
|
Have implemented the changes, except the points raised in #591 - But I agree that this should be addressed or (for now) removed before merge. |
more sky models, speed up module.
It should run exactly like before, just with the added option of being able to choose from more sky models during the begin() call. For some sky models, additional modules are necessary, but if they are not installed the module will default to the standard galaxy model.
The generation of sky powers was moved to begin() as otherwise the generation happens in every single run, slowing down the model considerably.