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 theta2 distribution plot to EventList class #1207

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@vuillaut
Contributor

vuillaut commented Nov 11, 2017

I added this simple plot that is used to check the distribution of the direction of the reconstructed events.
It also computes the 68% containment radius that I just print in the title.

@cdeil cdeil self-assigned this Nov 15, 2017

@cdeil cdeil added the feature label Nov 15, 2017

@cdeil cdeil added this to the 0.7 milestone Nov 15, 2017

@cdeil

@vuillaut - Thanks for the pull request! I left some inline comments.

Maybe the biggest question here is whether to allow the caller to choose different reference positions for the offset computation. There's two possible positions in the event file header that you could access, namely pointing and "object" position. The "object", i.e. target of observation is sometimes listed in the header and sometimes not.

One option is no center option and to use pointing position (what you do now). Another is to have e.g.

center = {'pointing', 'object'} or SkyCoord

i.e. use the pointing position by default, but give the caller the option to choose something else.

Anything here is fine with me, please choose what you find best, and make the docstring and implementation consistent.

Show outdated Hide outdated gammapy/data/tests/test_event_list.py Outdated
Show outdated Hide outdated gammapy/data/event_list.py Outdated
Show outdated Hide outdated gammapy/data/event_list.py Outdated
Show outdated Hide outdated gammapy/data/event_list.py Outdated

vuillaut and others added some commits Nov 15, 2017

corrected plot_theta2_distribution to use the more correct separation…
… method. Add optional center position as argument to be passed. Updated docstring and test accordingly
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 16, 2017

Member

@vuillaut - I've made a pull request against your pull request with one follow-up commit: vuillaut#1

This would be my suggestion. Note that I took out the R68 computation and instead explained in the docstring how to do it to the user. The problem with always computing and showing R68 is that it's often not what users want. E.g. I think this method can be used nicely to make theta^2 distributions wrt. the target position. But there computing R68 of all events in the FOV (2 deg in this case) is meaningless. So I would prefer to just add that via documentation to show users how to do it with 2 lines. Docstring now looks like this:
screen shot 2017-11-16 at 11 48 40

Thoughts?

Member

cdeil commented Nov 16, 2017

@vuillaut - I've made a pull request against your pull request with one follow-up commit: vuillaut#1

This would be my suggestion. Note that I took out the R68 computation and instead explained in the docstring how to do it to the user. The problem with always computing and showing R68 is that it's often not what users want. E.g. I think this method can be used nicely to make theta^2 distributions wrt. the target position. But there computing R68 of all events in the FOV (2 deg in this case) is meaningless. So I would prefer to just add that via documentation to show users how to do it with 2 lines. Docstring now looks like this:
screen shot 2017-11-16 at 11 48 40

Thoughts?

@vuillaut

This comment has been minimized.

Show comment
Hide comment
@vuillaut

vuillaut Nov 16, 2017

Contributor

Hi Christophe.
Yes I agree with you on the R68 computation, it might not be always required by the user.
The r68 computation could also be added as a function of the event list.

Contributor

vuillaut commented Nov 16, 2017

Hi Christophe.
Yes I agree with you on the R68 computation, it might not be always required by the user.
The r68 computation could also be added as a function of the event list.

Merge pull request #1 from cdeil/temp
Improve event list offset2 plot method a bit
@cdeil

cdeil approved these changes Nov 16, 2017

@cdeil cdeil changed the title from Adding theta2 distribution plot to EventList class to Add theta2 distribution plot to EventList class Nov 16, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 16, 2017

Member

I added one more commit f8cf94f and then merged this locally 1740a13

Usually I add commits to pull requests and merge via the Github web interface. But @vuillaut - it looks like you disallowed "edits from maintainers", which makes it harder to collaborate and integrate your pull requests. Would you mind to change to "allow edits from maintainers"?
https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

Member

cdeil commented Nov 16, 2017

I added one more commit f8cf94f and then merged this locally 1740a13

Usually I add commits to pull requests and merge via the Github web interface. But @vuillaut - it looks like you disallowed "edits from maintainers", which makes it harder to collaborate and integrate your pull requests. Would you mind to change to "allow edits from maintainers"?
https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

@cdeil cdeil closed this Nov 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment