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

Plotting circles #8

Merged
merged 3 commits into from
Mar 26, 2016
Merged

Plotting circles #8

merged 3 commits into from
Mar 26, 2016

Conversation

joleroi
Copy link
Contributor

@joleroi joleroi commented Mar 25, 2016

Example for plotting astropy.regions

@keflavich, @astrofrog

  • I don't know what API you had in mind (to_mpl()/plot() on each object vs. object_to_mpl(region)
  • I don't know how you want to do the plotting (transformation to pixels in astropy.regions then convert to patch vs. let matplotlib do the conversion (like I did for now) downside - always have to create wcsaxis instance)

To run an example go to regions/plotting and run

python plotting.py

@joleroi joleroi closed this Mar 25, 2016
@joleroi joleroi reopened this Mar 25, 2016
@joleroi joleroi changed the title plot circles Plotting circles Mar 25, 2016
output = list()

for reg in region_list:
temp = str(reg.__class__)
Copy link
Contributor

Choose a reason for hiding this comment

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

you could just use the region class directly, rather than the string name of the class

@keflavich
Copy link
Contributor

For API, I like to_mpl_patch for single regions, and to_mpl_patchcollection for multiple regions. It would be nice if we could have some sort of overloading that would allow matplotlib.pyplot.plot(region), but that is probably unrealistic. However, supporting matplotlib.axes.Axes.add_artist should be possible.

@astrofrog
Copy link
Member

+1 to to_mpl_patch and to_mpl_patchcollection

@joleroi
Copy link
Contributor Author

joleroi commented Mar 25, 2016

What about plotting SkyRegions? Ok to use WCSaxis?

@keflavich
Copy link
Contributor

yes, that's the right thing to do in examples, but that doesn't need to be part of the API (as long as the API is readily usable with wcsaxes)

@joleroi
Copy link
Contributor Author

joleroi commented Mar 25, 2016

I don't understand. Let met explain my problem again. This is how it's done in gammapy

def to_mpl_artist(self, ax, **kwargs):
        """Convert to mpl patch using a given wcs transformation
        Parameters
        ----------
        ax : `~astropy.wcsaxes.WCSAxes`
            WCS axis object
        kwargs : dict
            kwargs are forwarded to mpatches.Circle
        Returns
        -------
        patch : `~matplotlib.mpatches.Circle`
            Matplotlib patch object
        """

        import matplotlib.patches as mpatches

        val = self.pos.galactic
        center = (val.l.value, val.b.value)

        temp = dict(transform=ax.get_transform('galactic'),
                    radius=self.radius.value)
        kwargs.update(temp)
        patch = mpatches.Circle(center, **kwargs)

        return patch

So the WCSaxis is actually part of the API. My question is basically if I can copy&paste this function to astropy/regions or if we want to come up with another solution.

@keflavich
Copy link
Contributor

I understand better now. For now, yes, go with that. I'm not sure if there's a non-wcs-axis way to get the transforms easily.

@astrofrog
Copy link
Member

This looks great! Using WCSAxes sounds good to me.

@joleroi
Copy link
Contributor Author

joleroi commented Mar 25, 2016

I changed the API and added a basic test

@keflavich
Copy link
Contributor

OK, will review.

@keflavich
Copy link
Contributor

@joleroi say if you think this is ready to merge, or merge it yourself if you'd like. Others (@cdeil, @astrofrog, myself) are starting to hack on other aspects of the code. It would be nice to have this merged, but would also be nice to have it merged with passing tests.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7a2f152 on joleroi:ploting into * on astropy:master*.

@keflavich keflavich merged commit 960a6d2 into astropy:master Mar 26, 2016
@joleroi joleroi deleted the ploting branch May 2, 2016 18:05
@cdeil cdeil added this to the 0.1 milestone Jun 17, 2019
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 this pull request may close these issues.

None yet

5 participants