Skip to content
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

Cutout2D: Inconsistent array convention (needs documentation) #7414

Closed
msgordon opened this issue May 1, 2018 · 12 comments · Fixed by #7813
Closed

Cutout2D: Inconsistent array convention (needs documentation) #7414

msgordon opened this issue May 1, 2018 · 12 comments · Fixed by #7813
Labels
Docs Effort-low good first issue Issues that are well-suited for new contributors nddata Package-novice

Comments

@msgordon
Copy link

msgordon commented May 1, 2018

The ordered pair convention for Cutout2D is inconsistent and somewhat surprising. While the position argument is specified as (x, y), the size argument is reversed. From the documentation:

If size has two elements, they should be in (ny, nx) order.

While the (y, x) order is standard convention for most numpy/scipy functions, it is not intuitive to specify the position and size in reverse order from each other within the same function call.

@pllim pllim added the nddata label May 1, 2018
@pllim
Copy link
Member

pllim commented May 1, 2018

This might be one of those "the ship has sailed" kind of things, but I'll let @mwcraig , @crawfordsm , or @MSeifert04 chime in.

@bsipocz bsipocz added the Close? label May 1, 2018
@bsipocz
Copy link
Member

bsipocz commented May 1, 2018

Adding the label as per the comment above.

@mwcraig
Copy link
Member

mwcraig commented May 2, 2018

@larrybradley may have opinions here too...

@hamogu
Copy link
Member

hamogu commented May 13, 2018

I agree that it's probably too late to change, but at least we should make it more obvious in the docs that this is inconsistent. (And add it to the list of things we'll break if we ever do a python2->python3 style backwards-breaking step).

@pllim
Copy link
Member

pllim commented May 14, 2018

And add it to the list of things we'll break if we ever do a python2->python3 style backwards-breaking step

@hamogu , can you please clarify? What does x, y ordering have to do with PY2-PY3 transition? Also, we already removed PY2 support in master, so everything breaks in PY2 now unless you stick to v2.x series.

@hamogu
Copy link
Member

hamogu commented May 15, 2018

@pllim It has nothing to do with the py2-> py3 transition itself. All I'm saying is that this is something we might want to change if we ever do an astropy release that breaks backwards-compatibility.
There are packages in the Python ecosystem that found that they had accumulated so many inconsistencies that they just could not bear it any longer and then they made a big release that broke a lot of stuff. The prime example for the is "Python" itself - when Guido decided to launch the Python 3 series all packages that depend on Python (i.e. ALL python packages) need to adapt to that.

I don't know if we ever get to that point in astropy, maybe astropy 10.x? ;-)

@pllim
Copy link
Member

pllim commented May 15, 2018

Ah, gotcha! For now, I'll mark this as a documentation issue.

@pllim pllim changed the title Cutout2D: Inconsistent array convention Cutout2D: Inconsistent array convention (needs documentation) May 15, 2018
@pllim pllim added the good first issue Issues that are well-suited for new contributors label May 23, 2018
@astrofrog astrofrog added sprint and removed sprint labels May 23, 2018
@SG004
Copy link
Contributor

SG004 commented Sep 6, 2018

I am extraordinarily new at this, but I would like to make a pull request for this, if you'd let me.

@astrofrog
Copy link
Member

As an aside, I've made a new wiki page which we could use to track non-ideal APIs we are stuck with (in case we do ever have an opportunity to break things in future): https://github.com/astropy/astropy/wiki/Future-API-considerations

@astrofrog
Copy link
Member

@SG004 - yes feel free to open a pull request! As noted in the comments above, this is about clarifying things in the documentation not changing the behavior of Cutout2D

@larrybradley
Copy link
Member

To play devil's advocate 😈 , what would you want to change? Input positions (not indices!), especially for a 2D array, are commonly expected to be in (x, y) order and that agrees with the API of other packages (e.g. matplotlib, photutils). Similarly, shape or size parameters are usually expressed (e.g. scipy and numpy functions) in axis (e.g. (y, x)) order. I agree that this is a potential source of confusion and it should be very well documented, but I think the API is actually consistent with other packages/functions with regard to the inputs. I can't think of any other function offhand where both position and size/shape are inputs.

@msgordon
Copy link
Author

msgordon commented Sep 6, 2018

You're right that the position and shape orderings are not inconsistent with conventions in other libraries. However, the original reason for my post is that I can't remember any library where the orderings are mixed within the same function call. From the discussion here, it sounds like a potential solution is a very clear warning bubble or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs Effort-low good first issue Issues that are well-suited for new contributors nddata Package-novice
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants