Minor fixes for handling of older SMA data #1399
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1399 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 37 37
Lines 20763 20766 +3
=======================================
+ Hits 20747 20750 +3
Misses 16 16
Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
@kartographer -- It wasn't clear to me from the warning messages how the lst offset warnings and what to check are getting communicated back to the user.
pyuvdata/uvdata/mir.py
Outdated
| ): | ||
| if not np.allclose(val, mir_data.sp_data[item][data_mask]): | ||
| warnings.warn( | ||
| "Discrepancy in %s for win %i sb %i pol %i." % (item, *spdx) |
There was a problem hiding this comment.
Suggest adding some guidance in the warning message on whether this is a small discrepancy vs. something critical with the data.
Or would that be caught in the checks elsewhere?
There was a problem hiding this comment.
I extended the error message here in aa4bd2f -- the one time I saw this pop up (which used to throw an AssertionError) it was because a single record was bad, but it can also get triggered if there's a more significant issue to be checked. In the end I figured probably best to just point the user to what values should be looked at...
| if not np.allclose(lst_array, self.lst_array, rtol=0, atol=np.pi / 1728000.0): | ||
| # If this check fails, it means that there's something off w/ the lst values | ||
| # (to a larger degree than expected), and we'll pass them back to the user, | ||
| # who can inspect them directly and decide what to do. |
There was a problem hiding this comment.
This should get printed somewhere with suggestions on what to check.
There was a problem hiding this comment.
Okay, I've added a warning message here in aa4bd2f -- it's a little on the long side, although it hopefully gives enough guidance to the user on what to do and when to worry (it looks like the polling rate wasn't always 20 Hz, so this still triggers for some older data sets, but still in most cases it's just a problem with the metadata, not actually the visibilities).
Fixes a few issues that became apparent after #1371 was merged.
Description
There are a couple of changes that have been made here:
test_mir_parserthat were redundant have been removed.MirParser._make_v3_compliantwhere MJDs were not calculated correctly (forgot to convert UTC back to TT, which is the MIR standard).Mirdetermines whether or not a data set should be flex-pol.UVData.lst_arrayis now filled byset_lsts_from_time_arrayunder most circumstances.Motivation and Context
The change in flex-pol handling primarily affects reading in of older, pre-V3 MIR-formatted SMA data. Specifically, "gunnLO" was not consistently filled/used in pre-V3 data, whereas "fsky" has historically always been present, hence switching from the former to the latter.
The change in LST-handling arises from nuisance warnings about errors in the LSTs, which after #1356 have reduced somewhat in frequency for MIR data sets, but still regularly arise. I believe this is due to how these values are presently calculated, as a polled average rather than directly calculated at the integration mid-point, producing errors of up to 25 ms. Given that this is a long-standing feature, the reader now defaults to using pyuvdata-calculated LST values so long as they agree w/ the recorded values to within this precision limit. This has the advantage of providing "truer-to-observed" values while reducing the erroneous warnings. When the recorded values are not within the precision limit, they are plugged into
lst_arrayso that a warning is appropriately raised (and the issue can be handled by the user accordingly).Types of changes
Checklist:
Bug fix checklist: