Clarification / improved documentation on n_channels + banwidth parameters

[telegraphic]

Hello,

I was confused by this explanation in the Observation docstring:

n_channels (int, optional) – Number of channels used in the observation. Defaults to 82, which is equivalent to 1024 channels over 100 MHz of bandwidth. Sets maximum k_parallel that can be probed, but little to no overall effect on sensitivity.

bandwidth (float or Quantity, optional) – The bandwidth used for the observation, assumed to be in MHz. Note this is not the total instrument bandwidth, but the redshift range that can be considered co-eval.

Am I correct to take this as n_channels=82 across the co-eval bandwidth of bandwidth=8 MHz? (If so, the channel bandwidth is 97 kHz, which matches the aforementioned 1024 channels over 100 MHz)?

If this is right, may I suggest the docstring is updated to something like:

n_channels (int, optional) – Number of channels across the co-evaluated bandwidth (see 'bandwidth' parameter). Defaults to 82, to match the HERA 97 kHz channel width across 8 MHz. Sets maximum k_parallel that can be probed, but little to no overall effect on sensitivity.

bandwidth (float or Quantity, optional) – The bandwidth used for the observation, assumed to be in MHz. Note this is not the total instrument bandwidth, but the redshift range that can be considered co-evaluated.

Alternatively, these could be refactored so the user sets the channel_bandwidth instead, and bandwidth is renamed to coeval_bandwidth, which is a bit clearer (IMO).

[steven-murray]

HI @telegraphic -- yes, you're absolutely right that it means 82 channels across 8 MHz of "coeval" bandwidth. I think your suggested text is perfect.

I would consider allowing a new parameter channel_bandwidth which if given, over-rides the n_channels. We'd have to introduce it carefully so as not to break API though. We could also add coeval_bandwidth as an alias for bandwidth, but I think the docstring update you suggested does enough there.