Skip to content

Implement rotate method for more regions#285

Merged
cdeil merged 2 commits intoastropy:masterfrom
cdeil:moro
Jun 17, 2019
Merged

Implement rotate method for more regions#285
cdeil merged 2 commits intoastropy:masterfrom
cdeil:moro

Conversation

@cdeil
Copy link
Copy Markdown
Member

@cdeil cdeil commented Jun 17, 2019

In this PR I finish implementing #265 by adding a rotate for all pixel regions.

The only region it's missing for is compound, I'll add that in this PR now.

@keflavich @astrofrog or @larrybradley - How do you feel about adding PixelRegion.rotate, i.e. something to the base class?

It could be an abc.abstractmethod, forcing all pixel regions to implement this. For us in Gammapy where we call rotate on user-provided regions, that would be a good thing. I would prefer that. But it would also mean that everyone subclassing PixelRegion has to implement it, or put this if they don't want to:

def rotate(center, angle):
    raise NotImplementedError

I could also put a generic implementation, that works for most built-in regions, allowing us to remove lines, i.e. put it as PixelRegion.rotate as a non-abstract method:

def rotate(center, angle):
        changes = {}
        changes["center"] = self.center.rotate(center, angle)
        if hasattr(self, "angle"):
            changes["angle"] = self.angle + angle
        # could add more here: "vertices", "start", "end"
        # could also scan `self._repr_params for PixCoord and transform those
        return self.copy(**changes)

But I think this is bad style, to rely on region specific attributes in the base class, and to silently return the wrong thing if some pixel region is different?

@cdeil cdeil added this to the 0.4 milestone Jun 17, 2019
@cdeil cdeil self-assigned this Jun 17, 2019
@cdeil cdeil requested a review from keflavich June 17, 2019 07:54
@cdeil
Copy link
Copy Markdown
Member Author

cdeil commented Jun 17, 2019

I'll merge this now as-is, so that we can try it in Gammapy. Rotate now works for all pixel regions, and there are tests for all of them. I did not add PixRegion.rotate on the base class.

@cdeil cdeil merged commit b8154ab into astropy:master Jun 17, 2019
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.

2 participants