Support multiple spws in UVData.frequency_average
#1307
Conversation
bhazelton
changed the title
frequency_average method to support multiple spws and ragged averaging
bhazelton
changed the title
frequency_average method to support multiple spws and ragged averagingUVData.frequency_average
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1307 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 36 36
Lines 20142 20211 +69
=======================================
+ Hits 20126 20195 +69
Misses 16 16
Continue to review full report in Codecov by Sentry.
|
bhazelton
force-pushed
the
freq_avg_flex_spw
branch
from
b0627bf
to
4a52dda
Compare
bhazelton
force-pushed
the
freq_avg_flex_spw
branch
3 times, most recently
from
db8d06e
to
90b4a32
Compare
|
The test errors are due to deprecation warnings issued by numpy 1.25. Fixed in #1308 |
There was a problem hiding this comment.
Hey @bhazelton -- overall I think this is looking pretty good! One general comment, I think the frequency_average method could benefit from some additional comments within the code itself (there was a fair bit of jumping back and forth to try to understand the underlying logic of what was going on), but other than that mot much more than few small optimizations.
| this_final_reg_inds = final_spw_chans[spw] | ||
| if nfreq_mod_navg != 0: | ||
| # not an even number of final channels | ||
| regular_inds = these_inds[0 : n_final_chan_reg * n_chan_to_avg] |
There was a problem hiding this comment.
If may be nice to have some user-specified control on the channel at which you start averaging, particularly in the "ragged" case.
There was a problem hiding this comment.
I agree that it would be nice to allow the user to have more detailed control. We thought about something like you're suggesting, but as we thought about it seemed more complicated than just specifying where you start, e.g. what do you do with all the frequencies before the starting place? It seems to me that the most flexible thing would be to allow users to pass in an integer array that specifies which final channel to put each input channel into (and allowing the user to drop channels by specifying -1 in this array). But we decided to just do the obvious incremental improvement for now and wait to see what users request.
There was a problem hiding this comment.
We talked more about this on the telecon and decided that this kind of input array would make it fully flexible for the power user. I made an issue to this effect (#1311) so we don't lose track of the idea.
bhazelton
force-pushed
the
freq_avg_flex_spw
branch
from
90b4a32
to
18fc925
Compare
also update error messages now that frequency_average handles uneven freq spacing
bhazelton
force-pushed
the
freq_avg_flex_spw
branch
from
18fc925
to
2bcf934
Compare
I added a bunch of comments, let me know if they meet your needs. |
There was a problem hiding this comment.
Thanks @bhazelton, I think this looks good -- at least I found no glaring issues!
Description
This improves the
frequency_averagemethod in the following ways:respect_spwsparameter to control whether averaging can cross spectral window boundaries or not (default is True to disallow averaging over boundaries).Motivation and Context
closes #913
Types of changes
Checklist:
New feature checklist: