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 CTA Sensitivity class and plot improvements #831

Merged
merged 1 commit into from Jan 6, 2017
Merged

Conversation

@jjlk
Copy link
Contributor

@jjlk jjlk commented Dec 29, 2016

Hi @cdeil ,
Following this PR, gammapy/gammapy-extra#48, I added a class to handle the CTA sensitivity and I improved the plots. I modified the existing test accordingly.
Oké to merge?
Thanks

…ion to be able to compare performance
@cdeil cdeil self-assigned this Jan 6, 2017
@cdeil cdeil added the feature label Jan 6, 2017
@cdeil cdeil added this to the 0.6 milestone Jan 6, 2017
@cdeil cdeil mentioned this pull request Jan 6, 2017
5 of 5 tasks complete
@cdeil
Copy link
Member

@cdeil cdeil commented Jan 6, 2017

I'm merging this now, thanks!

Currently SensitivityTable sub-classes NDDataArray, which @joleroi is re-writing in #832 .
I don't know if using NDDataArray here is / will be a good idea.

@jjlk - In the future, please include at least one high-level test that exercises the main functionality. Without that, it becomes hard for us (in this case @joleroi in #832) to re-factor code, because it's not clear what the core functionality is that should be preserved. I presume in this case it's for now just to be able to read the file and call plot?

@cdeil cdeil merged commit a8bdda3 into gammapy:master Jan 6, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jjlk
Copy link
Contributor Author

@jjlk jjlk commented Jan 8, 2017

Currently SensitivityTable sub-classes NDDataArray, which @joleroi is re-writing in #832 .
I don't know if using NDDataArray here is / will be a good idea.

I'll adapt the classes (bkg, psf and sensitivity) when @joleroi's developments will be done.

@jjlk - In the future, please include at least one high-level test that exercises the main functionality. Without that, it becomes hard for us (in this case @joleroi in #832) to re-factor code, because it's not clear what the core functionality is that should be preserved.

Understood.

I presume in this case it's for now just to be able to read the file and call plot?

Yes. Thanks ++

@jjlk jjlk deleted the jjlk:cta_perf branch Jan 8, 2017
@joleroi
Copy link
Contributor

@joleroi joleroi commented Jan 9, 2017

I'll adapt the classes (bkg, psf and sensitivity) when @joleroi's developments will be done.

This is done in #832, so you don't have to worry. You might need to update some of your personal scripts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants