Improved UVData to MirParser interface #1282
Conversation
…g select capabilities phase on phase center name.
… metadata arrays.
… for MIR files, on in weights normalization for MIR
…file errors/missing records
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1282 +/- ##
========================================
Coverage 99.91% 99.91%
========================================
Files 33 33
Lines 19328 19459 +131
========================================
+ Hits 19312 19443 +131
Misses 16 16
Continue to review full report in Codecov by Sentry.
|
pyuvdata/uvdata/mir.py
Outdated
| self._add_phase_center( | ||
| mir_data.codes_data["source"][sou_id], | ||
| cat_type="sidereal", | ||
| cat_lon=np.median(icrs_ra), |
There was a problem hiding this comment.
For future proofing, I wonder about setting a check that np.ptp of the ra, dec varies less than some reasonable amount.
I think this would only matter for concatenated tracks (in which case, is that already handled when a phase center has the same name but different coordinates?).
There was a problem hiding this comment.
Fair point! I've added a check immediately after the code highlighted here, which raises a warning if a change of > 1 arcmin is seen (> 1 primary beam for SMA, which is pretty uncommon for standard sources).
| @@ -1144,6 +1154,8 @@ def get_value( | |||
| idx_arr = self._index_query(use_mask, where, and_where_args, header_key, index) | |||
|
|
|||
| if isinstance(field_name, (list, set, tuple)): | |||
| if return_tuples is None: | |||
There was a problem hiding this comment.
Why is the default changed to None? Can the kwarg in the function call just be True?
There was a problem hiding this comment.
So I'm using None here as a stand-in for what the typical usage is inside of MirParser, which is that if multiple fields have been requested, to return tuples (as they're often used for indexing via hashmap), but in cases where single field is requested, just to return the array of values. This is more of a convenience thing inside of MirParser rather than anything else -- it just helped to remove about two dozen extra keyword calls inside the code.
|
Thanks @e-koch! I think I've responded to everything you've commented on, but let me know if you see something that needs further attention. |
There was a problem hiding this comment.
This looks good to me. Thanks for all the hard work @kartographer!
Updates the interface between
UVData.readmethod and theMirParsermodule.Description
Overhauls the
read_mirmethod inside ofMir. This includes being able to sub-select desired data on read (rather than after-the-fact), with keywords matched to those typically used inside ofUVData.read. Additionally, some updates to theMirParserandMirmodules have been made to improve speed/memory performance (by up to a factor of two). TheUVData.readandUVData.selecthave also had thecatalog_nameskeyword added, which allows users to select sources/phase centers by their names rather than their ID numbers (the latter of which are sometimes arbitrarily assigned).Motivation and Context
This PR covers a final suite of changes that are needed for moving support for MIR data for general users of SMA -- in this case, primarily via improving the interface between the
UVDataand MIR-type files. Previously, in order to handle MIR files without loading the whole dataset first, one needed to pass forward specific indexing codes that were not necessarily known by lay-users of the telescope.After this PR is closed, I'd like to request a new pyuvdata version be spun up.
Types of changes
Checklist:
Bug fix checklist:
New feature checklist: