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

Imviz helper: rename offset_to to offset_byand have it use units #705

Closed
eteq opened this issue Jun 30, 2021 · 5 comments · Fixed by #709
Closed

Imviz helper: rename offset_to to offset_byand have it use units #705

eteq opened this issue Jun 30, 2021 · 5 comments · Fixed by #709

Comments

@eteq
Copy link
Contributor

eteq commented Jun 30, 2021

This is a simple follow-on from #687 - I failed to review in time, but fortunately had one-and-a-half minor change to sugges (i.e., just changes in offset_to):

  1. I think the offset_to helper method on Imviz 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)
@eteq eteq added the imviz label Jun 30, 2021
@pllim
Copy link
Contributor

pllim commented Jun 30, 2021

@pllim
Copy link
Contributor

pllim commented Jun 30, 2021

As for changing the API, I think it also has to happen in astrowidgets?

@eteq
Copy link
Contributor Author

eteq commented Jun 30, 2021

Yeah I suspect it was my fault 😉, but I do think this is a good opportunity to review these decisions.

I'd argue for this:

  1. We do the fix above in jdaviz now, but with a offset_to shim that respects the "old" API
  2. We make a fix in astrowidgets along the lines of what I was saying, with a deprecation warning for offset_to.

@eteq
Copy link
Contributor Author

eteq commented Jun 30, 2021

Although, maybe I should have reversed the order? I guess I could see it either way, depending on what you think @pllim

@pllim
Copy link
Contributor

pllim commented Jun 30, 2021

As discussed offline, I prefer the change to happen upstream first, so we do not set the precedence that it is okay to diverge from the astrowidgets API as we like.

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

Successfully merging a pull request may close this issue.

2 participants