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 region copy methods #269

Merged
merged 5 commits into from Jun 13, 2019

Conversation

@cdeil
Copy link
Member

commented Jun 10, 2019

As mentioned in #266 I'd like to add copy methods for regions.

I looked around a bit, e.g. here and here, and there's __copy__ and __deepcopy__ that we could implement.

However, looking at SkyCoord, that doesn't seem to be implemented there. So I'd suggest to follow that class, and add a copy method for regions.

@astrofrog @keflavich @larrybradley @sushobhana or anyone interested - could you please have a look at the one example method and test fr CirclePixelRegion.copy here? Is it OK, or would you suggest some different implementation or test pattern?

I'll wait a day here for suggestions before going ahead and adding copy methods for many region classes.

@cdeil cdeil added the enhancement label Jun 10, 2019

@cdeil cdeil added this to the 0.5 milestone Jun 10, 2019

@cdeil cdeil self-assigned this Jun 10, 2019

@larrybradley

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

LGTM, carry on!

@cdeil cdeil referenced this pull request Jun 12, 2019

@cdeil cdeil requested review from keflavich and larrybradley Jun 12, 2019

@cdeil cdeil modified the milestones: 0.5, 0.4 Jun 12, 2019

@cdeil

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

@keflavich or @larrybradley - Could you please review this?

I've added copy for circle, ellipse, rectangle and polygon here.
And would prefer to continue with other shapes in future PRs.

@cdeil

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

We could also implement copy in a generic way if we have a list of data fields for each class. And we could allow changes to be made on copy, which is very commonly what people want: make a copy, but change a few fields. I'm not sure if this generic copy written once in the base class would work for now, for the annuli classes (see #279).

See example implementing this proposal for one class: 93c3656

This is similar to what replace does for dataclasses

@cdeil

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

I'll merge this in now, because I've gone ahead and started to implement rotate (see #265) which requires this copy functionality. I'll open a new PR for that shortly where I also change the implementation of copy. Comments still welcome any time!

@cdeil cdeil merged commit 3011379 into astropy:master Jun 13, 2019

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.5%) to 92.0%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.