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

Sky region plot #221

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Sky region plot #221

wants to merge 1 commit into from

Conversation

cdeil
Copy link
Member

@cdeil cdeil commented Sep 15, 2018

In #76 (I think) I argued to remove the contains and plot method on SkyRegion objects, because it can be ambiguous how to transform the sky region to a pixel region and it's better to let the user be explicit:

sky_region.to_pix(wcs).plot(...)
sky_region.to_pix(wcs).contains(...)

Now using it myself, and seeing others plot many regions in Gammapy, I find it annoying to have to do sky_region.to_pix(wcs).plot(ax=ax) all the time, and would prefer sky_region.plot(ax=ax) to work directly and do whatever DS9 does to plot, presumably already what our to_pix(wcs).plot(ax=ax) does.

@astrofrog or anyone - is that possible? Does the ax have a link to the wcs when using wcsaxes, so that we can add a one-lineer in a method SkyRegion.plot in the base class to make this work?

Another issue I have with region plot is that if I call PixRegion.plot and then plt.legend, it never shows up in the legend. Only if I call region.as_artist and ax.add_artist does it show up in the legend. Does someone know if it's possible to plot regions and have them added to the legend in a simple way? Would be nice to extend the https://astropy-regions.readthedocs.io/en/latest/plotting.html page with an example.

@cdeil cdeil added this to the 0.4 milestone Sep 11, 2018
@keflavich
Copy link
Contributor

This sounds like it should be possible and I like the approach you've suggested for the skyregion plot api.

The second issue, with the legend, sounds like a bug, and I don't understand it.

@astrofrog
Copy link
Member

@cdeil - if you are using WCSAxes, then ax.wcs should give the WCS

@cdeil
Copy link
Member Author

cdeil commented Sep 15, 2018

@astrofrog - Thanks for the tip, implemented in 30eebc9 . If the current axis is a WCSAxes or one is passed in, calling plot on the sky region seems to work fine. I find this super convenient, it just does the right thing by default without having to pass the WCS all the time.

@astrofrog @keflavich @sushobhana - Please review the API / implementation.

Once it's clear what we want, I can add tests and docs.

@keflavich
Copy link
Contributor

The approach you've taken looks fine, that's what I'd do.

Copy link
Member

@sushobhana sushobhana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a plot, there should be as_artist method for sky regions?

@bsipocz bsipocz modified the milestones: 0.4, 0.5 Jun 10, 2019
@cdeil cdeil self-assigned this Jul 2, 2019
Base automatically changed from master to main March 17, 2021 18:42
@larrybradley larrybradley removed this from the 0.5 milestone Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants