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

rename offset_to to offset_byand have it use units #139

Closed
eteq opened this issue Jun 30, 2021 · 4 comments · Fixed by #140
Closed

rename offset_to to offset_byand have it use units #139

eteq opened this issue Jun 30, 2021 · 4 comments · Fixed by #140

Comments

@eteq
Copy link
Member

eteq commented Jun 30, 2021

This idea was triggered by looking at the implementation of another astrowidgets-conforming API in spacetelescope/jdaviz#705 :

  1. I think the offset_to method is better named offset_by. This aligns better with astropy.coordinates and I think it makes it just a bit clearer that you are giving the delta coordinates instead of actual coordinates
  2. I think we can dispense with the skycoord_offset keyword by instead using the units of dx/dy - if they are either pixel or dimensionless that can be interpreted as pixel offsets, and any other unit can assume SkyCoord/other WCS ("other wcs" can be implemented in the future, but I'm just trying not to accidentally specify ourselves out of it)

My proposal would be to keep in offset_to with its current API but have it start raising a deprecation warning (and internally it would call the new offset_by)

And yes, offset_to was my original suggestion. But I think it was wrong after seeing it in action!

@eteq
Copy link
Member Author

eteq commented Jun 30, 2021

Oh, and @pllim saw this idea already in spacetelescope/jdaviz#705, but @mwcraig do you have any thoughts pro- or anti- this?

@mwcraig
Copy link
Member

mwcraig commented Jun 30, 2021

I'm fine with this -- can it wait until we make some progress on the refactoring that @pllim started or does this need an implementation ASAP?

@pllim
Copy link
Member

pllim commented Jun 30, 2021

I don't foresee being able to refactor anything over here in the near future. Also see #131 (comment)

@pllim
Copy link
Member

pllim commented Jul 1, 2021

I see two different things proposed here. I think it is more practical to keep offset_to and offset_by separate; i.e:

  • offset_to: Deprecate. Keep old API.
  • offset_by: New API only.

(Maybe this was what you already proposed above... 🤷 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants