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

map.draw_rectangle API is inconsistent with submap #3325

Closed
Cadair opened this issue Aug 29, 2019 · 11 comments · Fixed by #3677
Closed

map.draw_rectangle API is inconsistent with submap #3325

Cadair opened this issue Aug 29, 2019 · 11 comments · Fixed by #3677
Labels
Effort Medium Requires a moderate time investment Feature Request New feature wanted! Hacktoberfest Issues that could be the focus of GSoC or Hacktoberfests map Affects the map submodule Package Novice Requires little knowledge of the internal structure of SunPy Priority High Rapid action required

Comments

@Cadair
Copy link
Member

Cadair commented Aug 29, 2019

submap uses top right and bottom left and draw rectangle uses bottom right width and height. This is stupid.

This is especially stupid as it prevents you from plotting a rectangle in the coordinates of one image over another.

@Cadair Cadair added Effort Medium Requires a moderate time investment Feature Request New feature wanted! map Affects the map submodule Package Novice Requires little knowledge of the internal structure of SunPy Priority High Rapid action required labels Aug 29, 2019
@Cadair
Copy link
Member Author

Cadair commented Aug 29, 2019

This will need careful API deprecation ☹️

@wtbarnes
Copy link
Member

wtbarnes commented Sep 4, 2019

What's the best way to go about this? Do we support an extra kwarg (top_right???) and ignore width and height if top_right is given?

@wtbarnes
Copy link
Member

wtbarnes commented Sep 4, 2019

Or do we add a new method altogether with this new syntax and then add a deprecation warning to draw_rectangle?

@Cadair
Copy link
Member Author

Cadair commented Sep 4, 2019

What's the best way to go about this? Do we support an extra kwarg (top_right???) and ignore width and height if top_right is given?

I was thinking this one.

I quite like the name draw_rectangle so I don't really know what I would call the new one.

@Cadair Cadair added the Hacktoberfest Issues that could be the focus of GSoC or Hacktoberfests label Sep 30, 2019
@Raahul-Singh
Copy link
Contributor

So, from what i get, we need to add a parameter top_right and use it and bottom_left to figure out height and width as plt.Rectangle works with height and width ?

@nabobalis
Copy link
Contributor

I assume we would want to unify the API for both functions using submap API's.

@Raahul-Singh
Copy link
Contributor

In other words, create the rectangle as is done in submap and use the same for draw_rectangle?

@nabobalis
Copy link
Contributor

Yeah but raise warnings if they use the old keywords that they will be removed in future.

@Raahul-Singh
Copy link
Contributor

Looking at the checks that my PR is failing, I think I'd need some assistance in cleaning this up. 😅

@nabobalis
Copy link
Contributor

Please try to keep PR problems to the PR and not the original issue.

@Raahul-Singh
Copy link
Contributor

Raahul-Singh commented Jan 11, 2020

Ok. Will keep that in mind 👍
May I delete the original comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort Medium Requires a moderate time investment Feature Request New feature wanted! Hacktoberfest Issues that could be the focus of GSoC or Hacktoberfests map Affects the map submodule Package Novice Requires little knowledge of the internal structure of SunPy Priority High Rapid action required
Projects
None yet
4 participants