Expand documentation for now APE14 WCS API by astrofrog · Pull Request #7962 · astropy/astropy

astrofrog · 2018-10-24T16:50:54Z

This adds some basic usage docs for the new WCS APE 14 API

One important addition - it implements WCS.array_shape which means that we'll finally be able to close e.g. #5461 or #4669 - at least I think.

Hey @drdavella - I really could do with scientific-python/pytest-doctestplus#2 here 😉 (not critical for the PR as I've just put +REMOTE_DATA everywhere, just pointing out a good use case!)

Once the CI runs, there will be a link to preview the docs (from 'giles') in the status checks below.

Direct link to docs preview

… in fact be used to access data

astropy-bot · 2018-10-24T16:50:58Z

Hi there @astrofrog 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

eteq

A few minor and a few larger suggestions. Note that I'm using this as an excuse to experiment with the new "suggest" thingie, so sorry if this ends up annoying @astrofrog.

eteq · 2018-10-24T18:59:21Z

+the method or property used, and this can normally be determined from the
+property or method name. Properties and methods containing ``pixel`` assume
+``(x, y)`` ordering, while properties and methods containing ``array`` assume
+``(row, column)`` ordering.


I find this a bit confusing. In my head "x" and "y" are the same thing as "row" and "column". Am I right that what we really mean is "the in-memory representation on the array" vs "the conceptual order of dimensions exposed to the user in the WCS itself"? That's a bit too wordy, but something in that direction? I think it's fine to use "row"/"column" vs "x"/"y", but only once you've defined what that means a bit more.

Would it help to say:

Properties and methods containing ``pixel`` assume ``(x, y)`` or ``Carstesian`` ordering, while properties and methods containing ``array`` assume ``(row, column)`` or ``numpy array`` ordering.

I guess that doesn't help because as our discussions revealed, I don't really get what "Cartesian ordering" means: to me "Cartesian" means "the metric is s^2 = x^2 + y^2", which is true regardless of the ordering. And "numpy array" also doesn't help because it can be both fortran or C ordered.

If preferred I can write the words on it... once I actually have right in my head which is supposed to be which 😉

I'll push some changes soon that are similar to @nden's suggestion.

eteq · 2018-10-24T19:08:43Z

+===========
+
+Let's take a look at the shared Python interface for WCS by using a spectral
+cube (two celestial axes and one spectral axis)::


This doesn't feel like "Basic" usage - a cube is pretty darn complex. What about starting with a simpler case of just an image and just show how world_to_pixel and pixel_to_world work (along with some of the informational bits you're demonstrating)? Then make this be "Advanced usage" or similar

eteq · 2018-10-24T19:28:12Z

One quick thing not directly related to the review: @nden - can you confirm the .array_shape as implemented here is compatible with/could be made so with gwcs (due to e.g. #5461 (comment))?

Co-Authored-By: astrofrog <[email protected]>

astrofrog · 2018-10-25T16:35:50Z

@eteq - I've expanded the pixel convention section. Does it make sense now? I also added a basic image example.

@nden - I added pixel_shape for consistency. Please feel free to do another PR to deprecate the naxis1/naxis2 access as you mentioned.

astrofrog · 2018-10-25T18:40:35Z

@eteq - I think this is ready for a final review/merge

nden · 2018-10-25T20:34:57Z

@eteq In gwcs I have a setter for array_shape because the interface to gwcs.WCS does not currently take a header like astropy.wcs.WCS. In the future, when there's a more user friendly interface this will change but I imagine the implementation will give the same results. The comment you linked to was referring to the equivalent of pixel_bounds. Now that I look at it, astropy.wcs.WCS.pixel_boundsis a property which returns None but is not settable. In gwcs pixel_bounds returns the bounding_box of the model. There's no equivalent attribute in astropy.wcs. So perhaps this property should have a setter in astropy?

nden · 2018-10-25T21:03:07Z

Here's a potential issue with not having setters for array_shape and pixel_shape.
There are setters for the private _naxis1 and _naxis2 attributes which we want to replace with the public pixel_shape. I can think of at least one use case where being able to set pixel_shape is useful. When drizzling images a common output WCS is created based on the WCSs of the input images. This output WCS needs to be assigned a pixel_shape value, normally computed using pixel_shape of all input WCSs.

astrofrog · 2018-10-25T21:14:00Z

@nden - we can easily add setters on the FITS WCS mix-in, but I think it would make more sense for you to do it in a separate PR as you know how best to set _naxis

nden · 2018-10-25T22:24:37Z

@astrofrog PR with setters

add shape setters

nden · 2018-10-26T01:15:04Z

@eteq I added pixel_bounds setter.

astrofrog · 2018-10-26T10:57:12Z

Ok so at this point this just needs a suggestion from @eteq on improving wording about the pixel convention stuff - but this can be merged without that change if it comes to it, as that can be clarified after feature freeze.

eteq · 2018-10-26T14:41:54Z

Yeah, I agree with @astrofrog that it's better to merge this and we can have a follow-on clarifying PR if/when we get to it. Presumably this wording works for some people. (At the very least, people that think like @nden and @astrofrog, so it's definitely better than nothing!)

astrofrog added 6 commits October 24, 2018 16:04

Implement array_shape in new WCS API for FITS-WCS

00c1a7f

Fix array indices returned from world_to_array_index so that they can…

720d014

… in fact be used to access data

Expanded documentation on new APE 14 WCS API

7f25e3d

Updated doctests

71a2122

Fix and test implementation of array_shape for FITS-WCS

94e6670

Make sure tests pass without --remote-data

dd9255b

astrofrog added Docs Enhancement no-changelog-entry-needed wcs.wcsapi labels Oct 24, 2018

astrofrog added this to the v3.1 milestone Oct 24, 2018

Check that data indexing does in fact work

821df44

astrofrog requested review from Cadair, eteq, keflavich and nden October 24, 2018 16:55

Fix intersphinx references

a17600b

eteq requested changes Oct 24, 2018

View reviewed changes

eteq and others added 6 commits October 25, 2018 09:01

Expand title of pixel convention section

a95eec2

Co-Authored-By: astrofrog <[email protected]>

Fix typo

be422be

Co-Authored-By: astrofrog <[email protected]>

Added overview

9d7d383

Co-Authored-By: astrofrog <[email protected]>

Expand documentation to include a simpler example to start with

08aa8b3

Added pixel_shape

6383537

Fix doctests

dd32e03

astrofrog added 2 commits October 25, 2018 18:07

Fix test

0558ef2

Fix Sphinx warning

3a63566

astrofrog added the Ready-for-final-review label Oct 25, 2018

nden approved these changes Oct 25, 2018

View reviewed changes

add shape setters

cc95b59

Merge pull request #75 from nden/shape-setters

e113924

add shape setters

nden mentioned this pull request Oct 25, 2018

deprecate _naxis1/2 #7973

Merged

larrybradley mentioned this pull request Oct 26, 2018

Update Cutout2D to use pixel_shape instead of _naxis #7978

Merged

eteq approved these changes Oct 26, 2018

View reviewed changes

eteq merged commit 4fc9f31 into astropy:master Oct 26, 2018

astrofrog deleted the ape-docs branch November 14, 2018 15:22

pllim mentioned this pull request Apr 6, 2020

Fixed reference to Greisen et al. 2002 in WCS API docs. #10118

Merged

Conversation

astrofrog commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astropy-bot Bot commented Oct 24, 2018

Uh oh!

eteq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eteq Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

nden Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

eteq Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

astrofrog Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

eteq Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

astrofrog Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eteq commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Oct 25, 2018

Uh oh!

astrofrog commented Oct 25, 2018

Uh oh!

nden commented Oct 25, 2018

Uh oh!

nden commented Oct 25, 2018

Uh oh!

astrofrog commented Oct 25, 2018

Uh oh!

nden commented Oct 25, 2018

Uh oh!

nden commented Oct 26, 2018

Uh oh!

astrofrog commented Oct 26, 2018

Uh oh!

eteq commented Oct 26, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

astrofrog commented Oct 24, 2018 •

edited

Loading

eteq commented Oct 24, 2018 •

edited

Loading