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 gammapy-spectrum-pipe #361

Merged
merged 20 commits into from Oct 14, 2015

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Sep 30, 2015

This PR introduces gammapy.scripts.spectrum_pipe, a simple but working pipeline for spectral analysis

The biggest downside is the lacking documentation, I would say

A side effect is some new functionality in gammapy.spectrum.spectrum_analysis (formerly scripts/spectrum_pipe)

  • Correctly write backscal fits keyword
  • Option to serialize fit output
  • Do not recreate OGIP files if not necessary
  • Make comparison plots to PA reference values
@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Oct 5, 2015

Contributor

at the moment OGIP files are only recreated if clobber is set to true in the config file or if the do not exist. If the user e.g. changes the ring size in the config file, it will have no effect if clobber is set to false. checking for an updated config file is not an option because we do not want to recreate the OGIP files if e.g. the spectral model has changed.
The easiest way is to trust the user to be smart enough to know when the has to recreate the files. What do you think?

Contributor

joleroi commented Oct 5, 2015

at the moment OGIP files are only recreated if clobber is set to true in the config file or if the do not exist. If the user e.g. changes the ring size in the config file, it will have no effect if clobber is set to false. checking for an updated config file is not an option because we do not want to recreate the OGIP files if e.g. the spectral model has changed.
The easiest way is to trust the user to be smart enough to know when the has to recreate the files. What do you think?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 5, 2015

Member

For now my suggestion would be to let the user declare via the config file which steps to run, and then to always run and clobber.

Make-like behaviour is hard too hard to implement in a good way quickly...

Member

cdeil commented Oct 5, 2015

For now my suggestion would be to let the user declare via the config file which steps to run, and then to always run and clobber.

Make-like behaviour is hard too hard to implement in a good way quickly...

@joleroi joleroi changed the title from WIP: Adding missing functionality to gammapy-spectrum-pipe to WIP: Introducing gammapy-spectrum-pipe Oct 7, 2015

@cdeil cdeil added the feature label Oct 7, 2015

@cdeil cdeil added this to the 0.4 milestone Oct 7, 2015

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 7, 2015

Member

@kingj90 - I know it's annoying to clean up old stuff, but could you please have a look at this?

I think the command line tool and docs page can be removed, it's broken and completely superseded by what you do here.

With the create_spectrum function I'm not sure ... from a 1 minute look I gather it did both reflected regions and some other background regions where it did acceptance corrections? If so it might be worth keeping in the repo as a code example for now, because some flexibility with the background region and acceptance correction when computing alphas is something we should do in the future.

Member

cdeil commented Oct 7, 2015

@kingj90 - I know it's annoying to clean up old stuff, but could you please have a look at this?

I think the command line tool and docs page can be removed, it's broken and completely superseded by what you do here.

With the create_spectrum function I'm not sure ... from a 1 minute look I gather it did both reflected regions and some other background regions where it did acceptance corrections? If so it might be worth keeping in the repo as a code example for now, because some flexibility with the background region and acceptance correction when computing alphas is something we should do in the future.

Show outdated Hide outdated gammapy/scripts/spectrum_pipe.py Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 13, 2015

Member

@kingj90 – Can we merge this PR?
(I'd like to start using this and it's easier if it's in master. You can always continue working on the extra things in new PRs. OK?)

Member

cdeil commented Oct 13, 2015

@kingj90 – Can we merge this PR?
(I'd like to start using this and it's easier if it's in master. You can always continue working on the extra things in new PRs. OK?)

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Oct 13, 2015

Contributor

I would like to upload some stuff tomorrow morning, I will merge this PR then, if thats in time for the release, If not feel free to merge yourself

Contributor

joleroi commented Oct 13, 2015

I would like to upload some stuff tomorrow morning, I will merge this PR then, if thats in time for the release, If not feel free to merge yourself

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 13, 2015

Member

OK, feel free to merge yourself tomorrow.

Member

cdeil commented Oct 13, 2015

OK, feel free to merge yourself tomorrow.

@joleroi joleroi changed the title from WIP: Introducing gammapy-spectrum-pipe to Introducing gammapy-spectrum-pipe Oct 14, 2015

joleroi added a commit that referenced this pull request Oct 14, 2015

Merge pull request #361 from kingj90/spectrum_pipev2
Introducing gammapy-spectrum-pipe

@joleroi joleroi merged commit 9d2e455 into gammapy:master Oct 14, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Oct 14, 2015

Contributor

Hi I will keep this branch, and apply the TODO not ticked yet, and also remove the old stuff you mentioned

Contributor

joleroi commented Oct 14, 2015

Hi I will keep this branch, and apply the TODO not ticked yet, and also remove the old stuff you mentioned

@joleroi joleroi deleted the joleroi:spectrum_pipev2 branch Oct 14, 2015

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 14, 2015

Member

I will keep this branch

Not sure what you mean.
You should start further work in a new branch, starting from master.
(Ignore me if this is just a misunderstanding and you know how you want to continue ...)

Member

cdeil commented Oct 14, 2015

I will keep this branch

Not sure what you mean.
You should start further work in a new branch, starting from master.
(Ignore me if this is just a misunderstanding and you know how you want to continue ...)

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Oct 14, 2015

Contributor

:D Forget what I say I though I could push a new local branch into the same remote branch and so keep this PR 'alive', but I guess I have to copy across the comments and so on I want to have in a new PR, right?

Contributor

joleroi commented Oct 14, 2015

:D Forget what I say I though I could push a new local branch into the same remote branch and so keep this PR 'alive', but I guess I have to copy across the comments and so on I want to have in a new PR, right?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 14, 2015

Member

I have to copy across the comments

Only if you think it's helpful for you. You can still look over the comments here and anyways the comments might not be very good because I haven't had time to use this code.

Member

cdeil commented Oct 14, 2015

I have to copy across the comments

Only if you think it's helpful for you. You can still look over the comments here and anyways the comments might not be very good because I haven't had time to use this code.

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Oct 14, 2015

Contributor

Alright!

Contributor

joleroi commented Oct 14, 2015

Alright!

@cdeil cdeil changed the title from Introducing gammapy-spectrum-pipe to Add gammapy-spectrum-pipe Oct 31, 2015

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 31, 2015

Member

I've moved the remaining open tasks from the task list here to #372.
Feel free to update the task list there as you like (remove what's no longer appropriate or split out into a separate issue if you don't want to address it before the 0.4 release Nov 11)

Member

cdeil commented Oct 31, 2015

I've moved the remaining open tasks from the task list here to #372.
Feel free to update the task list there as you like (remove what's no longer appropriate or split out into a separate issue if you don't want to address it before the 0.4 release Nov 11)

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