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

PIG 10 - Regions #2129

Merged
merged 10 commits into from Jun 17, 2019
Merged

PIG 10 - Regions #2129

merged 10 commits into from Jun 17, 2019

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented May 3, 2019

This PIG is to discuss and clarify the regions-related design, implementation and API questions.

It was triggered by PR #2089 by @bkhelifi - region rotation is the most complex region-related operation we have, and there one has to make the choice how to use pixel or sky regions.

Also there's quite a bit of cleanup of regions-related code to do in Gammapy, and probably some developments needed in astropy-regions and I think a PIG could with a task list could be useful to help get it done in the coming weeks and months. If someone wants to "champion" this work and make regions-related developments their topic until the summer, please comment here. This is also something that's quite easy to split and distribute among a few developers, as a series of small pull requests.

I will close the old issue #336 with this question how to handle regions in Gammapy now. It doesn't contain any valuable info or discussion.

I'll need a day or two two write this PIG, but if someone else would like to join and co-author it (i.e. write parts of it, not just review) - let me know. Of course anyone interested can and should review and comment in the coming weeks.

@cdeil cdeil added the pig label May 3, 2019
@cdeil cdeil added this to the 0.13 milestone May 3, 2019
@cdeil cdeil requested review from adonath, registerrier and bkhelifi May 3, 2019
@cdeil cdeil self-assigned this May 3, 2019
@cdeil cdeil added this to To Do in DOCUMENTATION via automation May 3, 2019
@cdeil cdeil mentioned this pull request May 3, 2019
@facero
Copy link
Contributor

@facero facero commented May 17, 2019

As discussed during the gammapy dev call this morning. Here is a simple example using Regions to extract a profile from a panda region using pyregion. It should be very similar with astropy-region.

for i in range(nsteps):
    Rin=Rout
    Rout=Rin+binsize.to('arcsec').value
    Rcenter.append( (Rout+Rin)/2 /3600 )
    region = 'fk5;panda(258.355 ,-39.771,%i,%i,1,%.2f",%.2f",1)'%(theta1,theta2,Rin,Rout)
    r = pyregion.parse(region)

    mask=r.get_mask(header=hdr,shape=(image.shape[0],image.shape[1]) ) 
    mask_cube[i,:,:]=mask
    
profile=(mask_cube*image).sum(axis=(1,2))  
plt.plot(Rcenter,profile)

docs/development/pigs/pig-010.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-010.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-010.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-010.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-010.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-010.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-010.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-010.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-010.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-010.rst Outdated Show resolved Hide resolved
@cdeil cdeil requested a review from adonath Jun 5, 2019
@cdeil
Copy link
Member Author

@cdeil cdeil commented Jun 5, 2019

@adonath @registerrier @facero @bkhelifi - I have finished a first complete draft of the PIG and just now pushed it in a72afbb .

Please have a look here: https://github.com/gammapy/gammapy/blob/a72afbb06497fb3e85a49b9a76b877d36787b021/docs/development/pigs/pig-010.rst

I would like to circulate this on Gammapy CC, Slack and mailing list officially asking for feedback soon, i.e. either tonight or tomorrow latest.

So if you have time to have a look today, or comment that you will review this early draft tomorrow, I'll wait for that and do one more round of improvements before.

@cdeil
Copy link
Member Author

@cdeil cdeil commented Jun 5, 2019

RST formatting issues fixed. Please look here.

Copy link
Contributor

@registerrier registerrier left a comment

Thanks @cdeil . Some comments regarding rotations and needed additions to regions.
Having an angular size for the region is also something important that could be done in the astropy-regions package.
Also a Region.copy() method would be very useful.

docs/development/pigs/pig-010.rst Show resolved Hide resolved
docs/development/pigs/pig-010.rst Outdated Show resolved Hide resolved
docs/development/pigs/pig-010.rst Show resolved Hide resolved
docs/development/pigs/pig-010.rst Show resolved Hide resolved
docs/development/pigs/pig-010.rst Show resolved Hide resolved
@cdeil
Copy link
Member Author

@cdeil cdeil commented Jun 6, 2019

Also a Region.copy() method would be very useful.

You mean to implement rotation as copy, followed by change of selected attributes, right?
I think you're right, that's the best way to implement it.

Or did you have some other use case for copy in mind?

@cdeil
Copy link
Member Author

@cdeil cdeil commented Jun 7, 2019

For region copy, I've opened astropy/regions#266 and added it to the task list of the PIG before astropy/regions#265

@cdeil
Copy link
Member Author

@cdeil cdeil commented Jun 7, 2019

@registerrier @adonath - thank you for all the feedback so far.

I'm circulating the PIG now more widely, with June 16 as deadline for comments.

The version to review is here: https://github.com/gammapy/gammapy/blob/09a7312eb87780afebdef77761def171ad3ce44d/docs/development/pigs/pig-010.rst

Unfortunately the PIG has gotten very long. Sorry for that. The most important part is the proposed task list, which you can see here.

Everyone interested in regions: please have a look and send feedback!

@cdeil cdeil removed the request for review from bkhelifi Jun 17, 2019
@cdeil
Copy link
Member Author

@cdeil cdeil commented Jun 17, 2019

I did not get any comments from https://groups.google.com/forum/#!topic/gammapy/dEBp6LbcwJ0 or the email to the CC list asking for review & feedback. Therefore I think we should merge the PIG now.
I've updated the status in 0389a9a

@registerrier @adonath - Please have a final look and either approve or ask for changes.

Work to implement the PIG has started. A pix region rotate method was added in https://astropy-regions.readthedocs.io/en/v0.4/changelog.html and discussion in PR #2242 is ongoing.

Overall I feel this PIG wasn't very useful, the main thing it provides is a description / reference what astropy-regions is, and our decision to work in the planar approximation like astropy-regions does and always go to pixel coordinates for computations. That last part wasn't clear to me, so for that I found it useful to have and discuss via a PIG.

Copy link
Contributor

@registerrier registerrier left a comment

Thanks @cdeil !

Copy link
Member

@adonath adonath left a comment

Thanks @cdeil. I have no further comments...

@cdeil cdeil merged commit 5af73cc into gammapy:master Jun 17, 2019
9 checks passed
DOCUMENTATION automation moved this from To Do to Done Jun 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
DOCUMENTATION
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants