You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
In developing the systematics simulation pipeline for H1C IDR3.2 validation, I've realized that there are a few improvements that could be made to the Reflections class. I'll try to explain here with the limited brain capacity I have left.
The docstring for the class (as well as the _complete_params method) doesn't accurately reflect the accepted types for the input parameters amp, dly, and phs. These can either be: None (use the default values), float (average value across the array), length-2 iterable (sample a uniform distribution bounded by the input), or array_like with the same length as the number of antennas (one per antenna).
Ensuring consistency of simulated parameters would be a lot more foolproof by allowing the parameters to be passed as a dictionary mapping antenna numbers to parameter values, as is the case for the cable_delays parameter in the OverAirCrossCoupling class.
To this end, I think I want to implement a somewhat API-breaking change to the Reflections class. I propose that the new parameters would be:
amp: float (avg across array) or dict (antnums -> values)
dly: same as above
phs: same as above
amp_bounds: (min, max) of uniform dist to sample
dly_bounds: same as above
phs_bounds: same as above
amp_jitter: keep as-is
dly_jitter: keep as-is
Although this breaks some of the current use-cases, I think it makes it harder to make a mistake when using this class. As it currently stands, I'm finding that I need to add some extra logic in my pipeline to ensure that the sorting of the antenna_numbers post-select agrees with the order that the reflection parameters are entered into the configuration file. (This is an issue because UVData.select on antennas changes the ordering of the antenna_numbers attribute, which is effectively what the Simulator refers to when it figures out what to pass to a class that asks for antenna numbers as a parameter.)
@steven-murray, I'd appreciate to hear your thoughts on this when you have some time. Definitely not a high priority item, but something that I think could be nice.
The text was updated successfully, but these errors were encountered:
Thanks for the feedback! I'll go with option (2) when I have some time.
Regarding other systematics classes that could use an update, the only thing that comes to mind right now is improving the models for bandpasses/beam areas. These are due for an update to H3C+, and I would like to support using Chebyshev and/or Legendre polynomials (I would actually prefer these over regular polynomials). On a related note, I've been wanting to improve the interpolators.py module for a while now. I'm not sure exactly how I'd like to improve it, but I think it could use a design review at the very least.
That said, I think these are relatively low-priority issues. Much higher up on my list of priorities is extending the Simulator to handle full visibility simulations and updating the quick-and-dirty visibility simulators to be based on the VisibilitySimulator class. I think that this is the big hurdle we need to clear in order to bring the package to the next level and allow for more natural support of the model proposed in #196.
In developing the systematics simulation pipeline for H1C IDR3.2 validation, I've realized that there are a few improvements that could be made to the
Reflections
class. I'll try to explain here with the limited brain capacity I have left._complete_params
method) doesn't accurately reflect the accepted types for the input parametersamp
,dly
, andphs
. These can either be:None
(use the default values),float
(average value across the array), length-2 iterable (sample a uniform distribution bounded by the input), orarray_like
with the same length as the number of antennas (one per antenna).cable_delays
parameter in theOverAirCrossCoupling
class.To this end, I think I want to implement a somewhat API-breaking change to the
Reflections
class. I propose that the new parameters would be:amp
: float (avg across array) or dict (antnums -> values)dly
: same as abovephs
: same as aboveamp_bounds
: (min, max) of uniform dist to sampledly_bounds
: same as abovephs_bounds
: same as aboveamp_jitter
: keep as-isdly_jitter
: keep as-isAlthough this breaks some of the current use-cases, I think it makes it harder to make a mistake when using this class. As it currently stands, I'm finding that I need to add some extra logic in my pipeline to ensure that the sorting of the
antenna_numbers
post-select agrees with the order that the reflection parameters are entered into the configuration file. (This is an issue becauseUVData.select
on antennas changes the ordering of theantenna_numbers
attribute, which is effectively what theSimulator
refers to when it figures out what to pass to a class that asks for antenna numbers as a parameter.)@steven-murray, I'd appreciate to hear your thoughts on this when you have some time. Definitely not a high priority item, but something that I think could be nice.
The text was updated successfully, but these errors were encountered: