Repository navigation
Conversation
|
ping @DanRyanIrish you might also end up interested in some of these 😀 |
dd14ba1 to
379cba8
Compare
379cba8 to
5392b5f
Compare
|
The first phase of this is ready - that is the classes and associated tests. For now there is no dispatch to specific types, but I wonder if we want to do this first, merge it once reviewed, then do a separate PR to figure out the dispatch? So I'm marking this as ready for review. |
|
I'm also tagging @eteq and @keflavich for review as original co-authors of APE 14 |
keflavich
left a comment
keflavich
left a comment
There was a problem hiding this comment.
I was only able to do a shallow review, but I don't see anything obviously wrong. A real review would entail trying these out somewhere, but I don't have use cases readily in hand.
Cadair
left a comment
Cadair
left a comment
There was a problem hiding this comment.
I think this is good and really want to see it in 4.1.
There is a bunch of commonality between these wrapper classes. For instance not all of them need to modify all the methods and properties, and all the __str__ and __repr__ are the same. Could we define a BaseWrapper ABC which just copies all the API from the _wcs object and then we can override it where needed?
Also I would like to see the __repr__'s changed to object.__repr__(self) + "\n" + str(self) so that they are different from the str and more importantly show what type they are, otherwise there is no way to tell the difference between all these classes sharing the same __str__ implementation.
| Parameters | ||
| ---------- | ||
| *wcs : `~astropy.wcs.wcsapi.BaseLowLevelWCS` | ||
| The WCSes to combine |
There was a problem hiding this comment.
This probably could do with a little more detail on ordering? I assume it's really pixel axes that we are in strict order here? Although obviously the world axes are not crossing the wcs lines at this point.
|
|
||
| @property | ||
| def world_axis_object_classes(self): | ||
| # TODO: deal with name conflicts |
There was a problem hiding this comment.
Should we at least detect this in the __init__ and throw a warning if we aren't going to fix it in the first release?
There was a problem hiding this comment.
Having played with this, is this comment even valid anymore?
|
Docs failures are related, but the rest might not, so please rebase during the next iteration. |
…ndex-related methods to the base classes since they can be effectively be the same for all implementations
…FITS and APE 14 WCS
[ci skip]
a0343e4 to
5836ec3
Compare
|
This needs new reviews as a lot has happened since the approval was given. |
…ound Implement pixel axis sharing in CompoundWCS
|
Hello @astrofrog 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.
|
There was a problem hiding this comment.
Can we add these to a document somewhere? I did try to go through and review, but I don't have the clearest idea of where and when I'm likely to want to use these wrappers. They're all meant to be low-level, but that means I'm probably the target audience as a user of the wrappers... should I be incorporating this into 1d spectra in SpectralCube (which should be inheriting from specutils anyway)?
I just don't remember what use case we raised this for. I should be able to figure this out.
|
We need to make the old import for |
This PR needs more work following discussions with @nden and @perrygreenfield
|
To prevent PR clutter, I'm going to close this as it's going to require more work and discussions with @nden and @perrygreenfield |
These APE 14 wrapper classes came out of discussions with @keflavich and @Cadair:
One unrelated but significant change here is changing
array_index_to_world_valuesandworld_to_array_index_valuesto have a default implementation, since there's really no reason those need to be defined every time (we end up copy/pasting a lot of code) and instead can just always use the equivalent 'pixel' methods as a starting point.We still need add a way for these classes to call out to implementation-specific alternatives (i.e. FITS WCS can implement these as pure FITS WCS modifications).