Refactor the telescope-related metadata in UVData, UVCal and UVFlag into an attached Telescope object

[bhazelton]

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

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation change (documentation changes only)
• Version change
• Build or continuous integration change

Checklist:

• I have read the contribution guide.
• My code follows the code style of this project.

New feature checklist:

• I have added or updated the docstrings associated with my feature using the numpy docstring format.
• I have updated the tutorial to highlight my new feature (if appropriate).
• I have added tests to cover my new feature.
• All new and existing tests pass.
• I have updated the CHANGELOG.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.93%. Comparing base (b54fce4) to head (f52fc0a).

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.
📢 Have feedback on the report? Share it here.

[steven-murray]

Thanks @bhazelton, this is going in some really good directions. I've made a number of comments, but I have two more overall comments:

1. Is there a good reason that Telescope.location is not an astropy Location (either EarthLocation or MoonLocation)? There's so much mucking around throughout the code with location_lat_lon_alt and location_lat_lon_alt_degrees etc, where all of this is already taken care of by this Location 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 the Location object). I consistently find this a sticking point when using pyuvdata -- 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.
2. 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.

[bhazelton]

@steven-murray I did add a new property on the Telescope object called location_obj which returns an EarthLocation (or a MoonLocation) so that a user can always just get the object. But I can look at making the location just be such an object, that might clean some things up.

I like the HDF5 idea, I had thought about doing something like that myself. I'll work on it.

[kartographer]

Just a small set of comments, nothing too major beyond what we talked about a couple of weeks ago.

[kartographer]

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.

[bhazelton]

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.

[steven-murray]

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.

[kartographer]

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).

[bhazelton]

@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.

[steven-murray]

Thanks @bhazelton -- this is very close I think. I did have a couple of stylistic comments, but should be easy to address.

[kartographer]

@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.

[bhazelton]

Astropy 6.1 dropped on pypi (but interestingly not yet on conda) and a new warning is being generated:

FutureWarning: Setting the location attribute post initialization will be disallowed in a future version of Astropy. Instead you should set the location when creating the Time object. In the future, this will raise an AttributeError.

I fixed a few places where we were causing this in pyuvdata, but most of them are coming from lunarsky, so I made an issue there (aelanman/lunarsky#27).

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 lunarsky.

[bhazelton]

@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.

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?

[steven-murray]

To add my two cents to some other issues brought up here (which we should move away into separate discussions...)...

1. I prefer the idea of a well-formed uvh5 file format where the telescope metadata is in its own sub-group that can be read completely independently. This would mean you could literally have hdf5 files with only telescope metadata in them and they would make sense, and could be read directly as Telescope objects. It's just more flexible. However, the old uvh5 file format should be supported forever. That is, there should be legacy UVH5 readers that know how to read older formats, and can read them perfectly fine as uvdata objects. My preferred way of doing this is to have separate reading classes/functions for different versions of the file format, and a dispatcher that knows which function to dispatch to given the file contents. I don't think we should have to worry about whether people are using older formats... the pyuvdata code itself should be how the files are read, and anyway each version of the file format should be documented, so people can use any format.
2. The movement of astropy away from setting attributes after object creation is unsurprising to me, and is how I (and I think most people) expect python objects to work. This is in contrast to pyuvdata, which is jarring to me every time I use it for this reason. It might be worth thinking about (for v4...) refactoring towards this modality.

[kartographer]

Thanks @bhazelton -- one very small piece of clean-up, otherwise I think this is looking good to go.