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

Add high-level docs for compound regions #74

Merged
merged 3 commits into from
Nov 17, 2016

Conversation

joleroi
Copy link
Contributor

@joleroi joleroi commented Nov 11, 2016

This PR

  • adds a short code example in the getting started section of the high-level docs to illustrate compound regions
  • adds a fancy image to the selfsame section

Ready for review

@joleroi joleroi added the docs label Nov 11, 2016
@joleroi joleroi added this to the 0.2 milestone Nov 11, 2016
@cdeil cdeil self-assigned this Nov 11, 2016
@cdeil
Copy link
Member

cdeil commented Nov 11, 2016

@joleroi - Thanks!

I put a version of the docs here:
https://www.mpi-hd.mpg.de/personalhomes/deil/public/regions/pr-74/getting_started.html#compounds

Currently (for me, on Python 3 with latest Astropy), the plot looks like this:
plot_compound

Should there be points outside the regions?
Or is there an effect (shift, center vs edge, WCS distortions) you hadn't intended?
I didn't look at the code that makes the plot yet, if you want to discuss it, come by my office.

Travis-ci fails are unrelated, I think:
https://travis-ci.org/astropy/regions/jobs/174955597#L743

Copy link
Member

@cdeil cdeil left a comment

Choose a reason for hiding this comment

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

Left two minor inline comments.

... radius=Angle('3 deg'),
... )
>>> type(circle1 & circle2)
... regions.core.compound.CompoundSkyRegion
Copy link
Member

Choose a reason for hiding this comment

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

Usually one puts stuff as it would appear in the Python console.
I.e. only put ... for input.
This line is output and should not have ... or be indented.
Same for print statement below.

@@ -0,0 +1,69 @@
import numpy as np
Copy link
Member

Choose a reason for hiding this comment

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

Please add a one-line description at the top of every new file.
People will copy & paste it and then it's good to have a one-line description if they find it in their Download folder a month later.
Something like:

"""
Example script illustrating compound regions.
"""

@joleroi
Copy link
Contributor Author

joleroi commented Nov 11, 2016

@cdeil I can hit Dismiss review ... very tempting!

@joleroi
Copy link
Contributor Author

joleroi commented Nov 11, 2016

Should there be points outside the regions?
Or is there an effect (shift, center vs edge, WCS distortions) you hadn't intended?

I don't think there should be points outside the regions. I check manually for the following point

<SkyCoord (Galactic): (l, b) in deg
    (358.0, 6.0)>

It is marked in red (and called strange point in the legend) in the plot
compound_debug

I am pretty sure that it is really and XOR point so the compound region contains method seems to work

In [12]: from astropy.coordinates import SkyCoord, Angle

In [13]: from regions import CircleSkyRegion

In [14]: # define 2 sky circles
    ...: circle1 = CircleSkyRegion(
    ...:     center=SkyCoord(1,2, unit='deg', frame='galactic'),
    ...:     radius=Angle('5 deg')
    ...: )
    ...:
    ...: circle2 = CircleSkyRegion(
    ...:     center=SkyCoord(-4,3, unit='deg', frame='galactic'),
    ...:     radius=Angle('3 deg'),
    ...: )
    ...:

In [15]: test_coord = SkyCoord(358, 6, unit='deg', frame='galactic')

In [16]: test_coord.separation(circle1.center) <= circle1.radius
Out[16]: array(True, dtype=bool)

In [17]: test_coord.separation(circle2.center) <= circle2.radius
Out[17]: array(False, dtype=bool)

So I guess there is some unwanted behaviour in the plotting method of CircleSkyRegion. @astrofrog @larrybradley - @cdeil said you're experienced with this kind of issues 😁. Any ideas?

@cdeil
Copy link
Member

cdeil commented Nov 11, 2016

Looks like a sky circle matplotlib plotting issue to me.
The script @joleroi wrote is OK, but the output isn't, as far as I can see.

@joleroi
Copy link
Contributor Author

joleroi commented Nov 14, 2016

The test failures are not related to changes in this PR, see e.g.
https://travis-ci.org/astropy/regions/jobs/175035799#L787
@astrofrog @larrybradley Ok to merge this and discuss the problem with the plotting in a separate issue/PR?

@cdeil cdeil merged commit f43434b into astropy:master Nov 17, 2016
@joleroi joleroi deleted the compound_docs branch November 21, 2016 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants