HERA-Team / hera_sim

Simple simulation code for HERA-like redundant interferometric arrays
Other
16 stars 8 forks source link

Improve sigchain.Reflections #198

Open r-pascua opened 2 years ago

r-pascua commented 2 years ago

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.

  1. 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).
  2. 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:

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.

steven-murray commented 2 years ago

This definitely makes sense to me. In terms of implementation, we have two options:

  1. Go ahead and make the change in a PR tagged API BREAKING and we can merge it in whenever we are ready to make the next version change.
  2. Make the change now but keep backwards compatibility with a deprecation warning and add a test that'll break on v3 (using the deprecations package).

I think (2) is better. While we're at it -- are there other systematics classes that could use a similar API update?

r-pascua commented 2 years ago

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.