Add Astropy view support to afw.table (DM-5641) #64
Conversation
| # | ||
|
|
||
| """ | ||
| Tests for Astropy views into afw.table Catalogs |
There was a problem hiding this comment.
Am I correct in thinking that these tests are not testing the new functionality in Astropy but are testing that the asAstropy method does the right thing. ie we are not (yet) depending on Astropy v1.2 for this to work. When v1.2 is released shouldn't we be able to instantiate an astropy.table directly with asAstropy being a private method? Or did I misunderstand?
There was a problem hiding this comment.
Basically correct. The tests do not exercise any new Astropy functionality. We do have a method that will hopefully work with v1.2, but that's untested.
But even once v1.2 is out, I'd like to keep the asAstropy (or whatever we call it) method public, as it provides useful customization options that will likely never be available through the Astropy table constructors (since they'd require API changes to those constructors).
There was a problem hiding this comment.
As I say above, I would prefer it if we just said "you pass this afw table to the astropy table constructor" rather than "you call this special exporter routine because it has an extra feature". Getting some extra **kwargs passed in to __astropy_table__ would seem feasible if we asked about it quickly. We'd then have a transitional APIs that would allow people to use table views before 1.2 astropy is released but with the expectation that in the future we'd use the astropy constructor.
There was a problem hiding this comment.
Oh, and my main comment here was that I'd like a comment in this test explaining that it's not actually testing the Astropy native constructor.
|
OK astropy/astropy#4885 is merged with |
We normally return read-only bool arrays when accessing Flag columns in Python, even though the arrays are copies and safe to edit, to prevent users from thinking they can modify Flag field values via the array interface. However, in an astropy.table view, users will generally asking for a copy, and in that case we don't want to restrict what they can do with it.
TallJimbo
force-pushed
the
tickets/DM-5641
branch
from
97242ba
to
54359a9
Compare
The __astropy_table__ method is called by the Astropy Table constructor (in Astropy >= v1.2) to support the syntax: t = astropy.table.Table(catalog) We also provide our own direct interface that returns an Astropy Table (or QTable) as an "asAstropy" method.
TallJimbo
force-pushed
the
tickets/DM-5641
branch
from
54359a9
to
44cac51
Compare
| def __astropy_table__(self, cls, copy, **kwds): | ||
| """Implement interface called by Astropy table constructors to construct a view or copy. | ||
| """ | ||
| return _syntax.BaseCatalog_asAstropy(cls=cls, copy=copy) |
There was a problem hiding this comment.
Shouldn't the kwargs have been forwarded on here?
There was a problem hiding this comment.
Thought I did that, but apparently not. I'll create a mini-ticket to fix.
This adds an
asAstropymethod to get aastropy.table.Tableorastropy.table.QTableview directly, as well as an__astropy_table__that will allow the Astropy table constructors to accept LSSTCatalogobjects (once astropy/astropy#4885 is complete).The astropy PR has not quite been finalized, but I'm confident that any further changes will be minor, and hence it should be fine to review this now. I will delay the merge of this ticket until the astropy PR is finalized (they also have a feature freeze this weekend, so I don't think we'll have to wait long).