-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
^ uvh5 addition #1511
^ uvh5 addition #1511
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1511 +/- ##
========================================
Coverage 99.93% 99.93%
========================================
Files 63 63
Lines 21834 22066 +232
========================================
+ Hits 21819 22051 +232
Misses 15 15 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment, and one additional request -- now that we have lunarsky patched, I can see that there's an unfiltered warning from the test that was merge in #1506. Could you just add Looks like this was already taken care of!@pytest.mark.filterwarnings("ignore:The uvw_array does not match the expected values")
above the test in question?
Also, make sure to update the CHANGELOG =)
src/pyuvdata/uvdata/uvdata.py
Outdated
self.phase_center_catalog[cat_id]["info_source"] = cat_entry["info_source"] | ||
info_source = cat_entry.get("info_source") | ||
if info_source is not None: | ||
self.phase_center_catalog[cat_id]["info_source"] = info_source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to:
self.phase_center_catalog[cat_id]["info_source"] = cat_entry.get("info_source")
Realizing this is pretty minor, although the plan for the phase dict is to become an actual object, where these will become optional attributes (which always exist as attributes on the object, but are filled in w/ None when not set), and this will basically make the code easier to migrate.
@radonnachie - also, I think the test files are also sufficiently small enough that the additions should be totally fine. I'd just say make sure those are the test files you want to include (but very happy to add some ATA data finally to the list of test datasets!). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
ed71586
into
RadioAstronomySoftwareGroup:main
Patched a minor bug that arose in the addition of two uvh5 objects at the ATA.
Description
The catalog entries were expected to have the optional "info_source" key-values. Instead, this optionally retrieved and processed if not
None
.Motivation and Context
Types of changes
Checklist:
Bug fix checklist:
New feature checklist:
Breaking change checklist:
Documentation change checklist:
Version change checklist:
Build or continuous integration change checklist: