Rename Mask class and add origin arg to plot methods #203

sushobhana · 2018-08-12T18:32:33Z

According to discussion in astropy/photutils#684

Added origin arg to plot methods (which is present in photutils)
Added to_region and plot methods to BoundingBox
Rename Mask to RegionMask.

Now, RegionMask are ApertureMask are same both API and code wise.
Geometry functions and BoundingBox are also same in both packages.

keflavich

The PR looks good to me. It would be great if you could include a document example with a figure showing how and when you'd use the offset origin option. Maybe on cropped data?

keflavich · 2018-08-13T15:32:47Z

regions/core/core.py

        """
        Calls as_patch method forwarding all kwargs and adds patch
        to given axis.

        Parameters
        ----------
+        origin : array_like, optional
+            The ``(x, y)`` position of the origin of the displayed image.


could you clarify: these are x,y positions in pixel units, right? Just to make sure they're not confused with data units (e.g., WCS)

yes, pixel units. Perhaps should we accept PixCoord instead of a tuple to avoid confusion?

That's... a good idea from a programmer's perspective, as it makes the code more robust, but it's really inconvenient for something as simple as a pixel coordinate, since it requires an additional import and a much longer function call. I am weakly in favor of accepting a tuple and just documenting it.

keflavich · 2018-08-13T15:32:57Z

regions/shapes/circle.py

+        Parameters:
+        -----------
+        origin : array_like, optional
+            The ``(x, y)`` position of the origin of the displayed image.


same clarification here

keflavich · 2018-08-13T15:33:28Z

regions/shapes/ellipse.py

+        Parameters:
+        -----------
+        origin : array_like, optional
+            The ``(x, y)`` position of the origin of the displayed image.


and here, and in all similar instances

cdeil · 2018-08-21T09:54:34Z

@sushobhana - Please rebase against upstream master to resolve the merge conflicts.

sushobhana · 2018-08-26T17:54:24Z

@keflavich Added an example and rebased it. Ready to merge??

keflavich

Needs some docstring cleanup, then I think it's good?

keflavich · 2018-08-27T15:21:01Z

docs/plotting.rst

+You can shift the origin of the region very conveniently while plotting by simply
+supplying the ``origin`` pixel coordinates to :meth:`~regions.PixelRegion.plot`
+and :meth:`~regions.PixelRegion.as_patch`. The ``**kwargs`` argument takes any
+keyword argument that the `~matplotlib.patches.Patch` object accepts. For Ex:


either say "For example:" or "e.g.:"

keflavich · 2018-08-27T15:53:39Z

docs/plotting.rst

-`~regions.CompundPixelRegion` don't have because there are no equivalent classes
-in `matplotlib.patches`.
+the `~regions.TextPixelRegion` and `~regions.CompoundPixelRegion`
+don't have because there are no equivalent classes in `matplotlib.patches`.


don't have what? don't have as_patch methods?

It is mentioned in the above line

yes, but this is grammatically incorrect.

I rephrased it.

keflavich · 2018-08-27T16:17:17Z

regions/core/bounding_box.py

+        ax : `matplotlib.axes.Axes` instance, optional
+            If `None`, then the current `~matplotlib.axes.Axes` instance
+            is used.
+        kwargs


will this render correctly? I think you need kwargs: dict ?

keflavich · 2018-08-27T16:17:33Z

regions/core/bounding_box.py

+        origin : array_like, optional
+            The ``(x, y)`` position of the origin of the displayed
+            image.
+        ax : `matplotlib.axes.Axes` instance, optional


whitespace is inconsistent; no before :

actually, sphinx doesn't render properly if there is no before :

Oh... my bad, you have to add spaces in other places, not remove them here

keflavich · 2018-08-27T16:18:24Z

regions/core/compound.py

+        """
+        Matplotlib patch object for annulus region (`matplotlib.patches.PathPatch`).
+
+        Parameters:


no trailing :

sushobhana · 2018-08-27T17:43:05Z

Thanks @keflavich !

keflavich · 2018-08-27T19:38:27Z

docs/plotting.rst

@@ -34,7 +34,7 @@ and calls ``plt.gca()`` if no axis is passed in.
 You can shift the origin of the region very conveniently while plotting by simply
 supplying the ``origin`` pixel coordinates to :meth:`~regions.PixelRegion.plot`
 and :meth:`~regions.PixelRegion.as_patch`. The ``**kwargs`` argument takes any
-keyword argument that the `~matplotlib.patches.Patch` object accepts. For Ex:
+keyword argument that the `~matplotlib.patches.Patch` object accepts. For e.g.:


it's just e.g., not For e.g., but I'll handle that

I did not know e.g. itself means for example, I thought it is short for example (had to google it!) ..hehe

keflavich approved these changes Aug 13, 2018

View reviewed changes

sushobhana mentioned this pull request Aug 23, 2018

regions version 0.3 #125

Closed

cdeil added this to the 0.3 milestone Aug 24, 2018

cdeil assigned sushobhana Aug 24, 2018

cdeil added the enhancement label Aug 24, 2018

sushobhana added 3 commits August 26, 2018 19:17

added to_region and plot methods for bounding box

f3fbd58

added origin arg for as_patch and plot methods

b44e99a

renamed Mask class to RegionMask

75c8692

sushobhana force-pushed the photutils branch from fab8eba to 75c8692 Compare August 26, 2018 13:53

sushobhana added 5 commits August 26, 2018 19:42

explicitly explaining the origin arg in plot and as_patch method

4181fd0

setting the default mpl param values in as_patch method

70b388b

added examples to show the shift in origin

7448596

plot method returns the axes

92bc2fb

minor typo: axes instead of axis

1567ac5

cdeil assigned keflavich and unassigned sushobhana Aug 26, 2018

minor change in the doc example

854cd32

sushobhana force-pushed the photutils branch from 8ed7ea6 to 854cd32 Compare August 26, 2018 18:00

keflavich reviewed Aug 27, 2018

View reviewed changes

minor docstring fixes

b5237c5

sushobhana force-pushed the photutils branch from 0e069d3 to b5237c5 Compare August 27, 2018 17:50

keflavich reviewed Aug 27, 2018

View reviewed changes

keflavich merged commit b5237c5 into astropy:master Aug 27, 2018

larrybradley added feature-request and removed enhancement labels Mar 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename Mask class and add origin arg to plot methods #203

Rename Mask class and add origin arg to plot methods #203

sushobhana commented Aug 12, 2018 •

edited

keflavich left a comment

keflavich Aug 13, 2018

sushobhana Aug 13, 2018

keflavich Aug 13, 2018

keflavich Aug 13, 2018

keflavich Aug 13, 2018

cdeil commented Aug 21, 2018

sushobhana commented Aug 26, 2018

keflavich left a comment

keflavich Aug 27, 2018

keflavich Aug 27, 2018

sushobhana Aug 27, 2018

keflavich Aug 27, 2018

sushobhana Aug 27, 2018

keflavich Aug 27, 2018

keflavich Aug 27, 2018

sushobhana Aug 27, 2018

keflavich Aug 27, 2018

keflavich Aug 27, 2018

sushobhana commented Aug 27, 2018

keflavich Aug 27, 2018

sushobhana Aug 27, 2018

Rename Mask class and add origin arg to plot methods #203

Rename Mask class and add origin arg to plot methods #203

Conversation

sushobhana commented Aug 12, 2018 • edited

keflavich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeil commented Aug 21, 2018

sushobhana commented Aug 26, 2018

keflavich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushobhana commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushobhana commented Aug 12, 2018 •

edited