-
Notifications
You must be signed in to change notification settings - Fork 25
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
Refactor the telescope-related metadata in UVData, UVCal and UVFlag into an attached Telescope object #1422
Conversation
084a0c3
to
bfb65e8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## prep_v3.0 #1422 +/- ##
===========================================
Coverage 99.92% 99.93%
===========================================
Files 41 41
Lines 22406 22702 +296
===========================================
+ Hits 22390 22687 +297
+ Misses 16 15 -1 ☔ 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.
Thanks @bhazelton, this is going in some really good directions. I've made a number of comments, but I have two more overall comments:
- Is there a good reason that
Telescope.location
is not an astropyLocation
(eitherEarthLocation
orMoonLocation
)? There's so much mucking around throughout the code withlocation_lat_lon_alt
andlocation_lat_lon_alt_degrees
etc, where all of this is already taken care of by thisLocation
object. Now would be the perfect time to move to such a representation (the attributes on the top-level classes can remain the same for now, since they can be computed automatically from theLocation
object). I consistently find this a sticking point when usingpyuvdata
-- I never quite know where the telescope location information exists, and where to set it, and which properties will need to be manually updated if I change it, etc. - I think we should consider having a
.write_hdf5()
method on the Telescope object, which can write out to a Group, irrespective of any upper class to which it might belong (UVData, UVCal etc.). This would make it more consistent to read the Telescope object from any file, and can be called directly from the upper method in their write methods.
@steven-murray I did add a new property on the Telescope object called I like the HDF5 idea, I had thought about doing something like that myself. I'll work on it. |
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.
Just a small set of comments, nothing too major beyond what we talked about a couple of weeks ago.
@@ -683,6 +634,140 @@ def __init__( | |||
"list, tuple, string, pathlib.Path, UVData, or UVCal." | |||
) | |||
|
|||
def _set_telescope_requirements(self): |
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.
Given that this is going in during a major version change, do we want to make Telescope
act consistently across various UVBase
objects? Having to account for under-the-hood attributes when sharing a common attribute seems like it might be an ingredient for things breaking in subtle and unexpected ways.
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.
I think the real issue comes down to x_orientation
as we discussed on the telecon. We can't really require it for UVData because many existing file formats do not have it or require it and there's no good way to automatically fill it in. But we do require it on UVCal. Unless we decide to back off on that requirement, we're stuck with these differences. I did make the things that are always required on all objects always be required on Telescope and removed those from these methods to make it clearer what the differences are.
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 probably not very satisfying way of solving this would be to require x_orientation
everywhere, and force people to pass it into the read()
method for file formats that don't include it.
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.
Okay, I think I'm content to leave it be for now -- x_orientation
is probably where this runs aground, but I realized that I had some ideas on how to retire this (as part of addressing #854), and maybe this could be just left as is for now (though instrument
could probably be made to be always optional -- actually not sure why it's required for UVData
at the moment).
25ba0e0
to
a27a89b
Compare
a0b11b4
to
308ebcc
Compare
766dec6
to
60a2c2a
Compare
@steven-murray @kartographer I think this ready for another look when you have time. I think I've addressed all your comments and what we discussed in the telecons. |
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.
Thanks @bhazelton -- this is very close I think. I did have a couple of stylistic comments, but should be easy to address.
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.
@bhazelton looks good overall -- I don't have any detailed comments beyond what @steven-murray has already marked, the only thing that I think would be worth discussing is what will happen when we have new parameters to add to the Telescope
object when reading/writing to HDF5-formatted files (this is of particular interest for SMA, since I've got some parameters already in mind that I'll start working on once v3 is done). Right now, everything is getting written to the main header, and I don't know if that's what we want to keep on doing, or if we want to have the new parameters grouped in some sensible way. I did this in an ad-hoc fashion w/ phase_center_catalog
, which of course worked okay but changed shortly after that was integrated, and so if we don't want to trust my on-the-fly approach (which I never do 😉) it might be good to discuss how we might do this in the future and make some comments in the code accordingly.
Astropy 6.1 dropped on pypi (but interestingly not yet on conda) and a new warning is being generated:
I fixed a few places where we were causing this in pyuvdata, but most of them are coming from This is causing our warnings tests to fail. Not sure if I want to limit astropy on that vs just waiting for it to be fixed on |
Good question. I'd really like this to be a larger conversation as it pertains to the UVH5 format which is being used beyond pyuvdata at this point. Currently in that format we really only differentiate between header and data items, where anything that's not the size of visibility data are lumped in the header. The phase center catalog is also in header, although it is a nested dataset. On the one hand, I understand the niceness of grouping telescope related data within a dataset, especially as we are now representing it that way internally to pyuvdata, but on the other hand since the format is more widely used I don't know that we should force our object hierarchy on the format too strictly. Maybe @plaplant or @david-macmahon have thoughts? |
To add my two cents to some other issues brought up here (which we should move away into separate discussions...)...
|
66ec5a8
to
4836be1
Compare
Fixing the tols led to many fewer warnings in tests.
exposed in pyuvsim tests
6370d76
to
4676111
Compare
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.
Thanks @bhazelton -- one very small piece of clean-up, otherwise I think this is looking good to go.
Description
All the metadata which is really about the telescope (rather than the dataset) has been pulled out into a Telescope object which is then attached to each object as an attribute. I used
__getattr__
and__setattr__
on UVBase to keep the old API (but with deprecation warnings). I think other the biggest difference a user might notice is that some new metadata can show up on UVCal and UVFlag (e.g. instrument and antenna_diameters).This really streamlines a bunch of code and will make it easier to maintain.
Motivation and Context
closes #1095
Types of changes
Checklist:
New feature checklist: