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

Restructure spectrum package and command line tool #436

Merged
merged 23 commits into from Feb 9, 2016

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Feb 6, 2016

this PR changes

the structure of gammapy.spectrum

  • SpectrumObservation is now a pure container class with classmethods doing the work
  • SpectrumAnalysis -> SpectrumExtraction. Only algorithms
  • Add SpectrumObservationList to represent a list of observation. Dummy methods for stacking
  • Fitting is completely independent now and lives in spectrum.spectrum_fit
  • Add utility function to spectrum.result for overplotting and comparison tables

the structire of the gammapy-spectrum command line tool

  • Add subcommands extract fit display plot

@cdeil cdeil changed the title from Restructure gammapy.spectrum and gammapy-spectrum command line tool to Restructure spectrum package and command line tool Feb 6, 2016

@cdeil cdeil added this to the 0.4 milestone Feb 6, 2016

@joleroi joleroi self-assigned this Feb 6, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 6, 2016

Member

@joleroi - Now that you've made a change in gammapy-extra, but this PR from gammapy hasn't landed yet, the build is broken:
https://travis-ci.org/gammapy/gammapy/jobs/107459817#L885

I really need to change the setup, so that gammapy-extra is a git submodule ... this will give us versioning for gammapy-extra and sanity.

Member

cdeil commented Feb 6, 2016

@joleroi - Now that you've made a change in gammapy-extra, but this PR from gammapy hasn't landed yet, the build is broken:
https://travis-ci.org/gammapy/gammapy/jobs/107459817#L885

I really need to change the setup, so that gammapy-extra is a git submodule ... this will give us versioning for gammapy-extra and sanity.

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Feb 6, 2016

Contributor

@cdeil Sorry for breaking everything 😁

Can you please have a look at gammapy/spectrum/results.py (and for now only at this). There are not test yet (only one example) but next I want to add a test that runs an entire crab analysis and does stuff with the results, so the results classes will be tested in this high-level test (You said: High-level tests over unit-tests).

Is there any functionality in spectrum/spectrum_results.py that you need and is not covered in the new stuff? Otherwise I would think about removing it, because having spectrum_results.py and results.py is confusing (Luigi already stumbled over this).

Contributor

joleroi commented Feb 6, 2016

@cdeil Sorry for breaking everything 😁

Can you please have a look at gammapy/spectrum/results.py (and for now only at this). There are not test yet (only one example) but next I want to add a test that runs an entire crab analysis and does stuff with the results, so the results classes will be tested in this high-level test (You said: High-level tests over unit-tests).

Is there any functionality in spectrum/spectrum_results.py that you need and is not covered in the new stuff? Otherwise I would think about removing it, because having spectrum_results.py and results.py is confusing (Luigi already stumbled over this).

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Feb 6, 2016

Contributor

You only want to do the tutorial once this PR is merged, right?

Contributor

joleroi commented Feb 6, 2016

You only want to do the tutorial once this PR is merged, right?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 6, 2016

Member

Can you please have a look at gammapy/spectrum/results.py (and for now only at this).

I had a quick look. It's super-hard to review this in isolation and give useful feedback.

I'd much prefer to merge in master, and let me just use this for an hour tomorrow and look around the docs and code. Then I can give suggestions like I did in #419.

@joleroi - Would this be OK for you?

You said: High-level tests over unit-tests

Yes, I don't think unit tests make sense as long as the API is still heavily changing. That's not just me ... you can find many opinions that unit tests don't make sense during prototyping, because you'd spend so much time refactoring the tests over and over as the API changes.

Is there any functionality in spectrum/spectrum_results.py that you need and is not covered in the new stuff?

I'm not sure ... could you leave it in for now and I'll have a detailed look tomorrow?

Member

cdeil commented Feb 6, 2016

Can you please have a look at gammapy/spectrum/results.py (and for now only at this).

I had a quick look. It's super-hard to review this in isolation and give useful feedback.

I'd much prefer to merge in master, and let me just use this for an hour tomorrow and look around the docs and code. Then I can give suggestions like I did in #419.

@joleroi - Would this be OK for you?

You said: High-level tests over unit-tests

Yes, I don't think unit tests make sense as long as the API is still heavily changing. That's not just me ... you can find many opinions that unit tests don't make sense during prototyping, because you'd spend so much time refactoring the tests over and over as the API changes.

Is there any functionality in spectrum/spectrum_results.py that you need and is not covered in the new stuff?

I'm not sure ... could you leave it in for now and I'll have a detailed look tomorrow?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 7, 2016

Member

+1,937 −1,409 lines -> "looks fine" :-)

Member

cdeil commented Feb 7, 2016

+1,937 −1,409 lines -> "looks fine" :-)

joleroi added some commits Feb 8, 2016

@joleroi joleroi referenced this pull request Feb 9, 2016

Closed

Grouping2 #429

joleroi added some commits Feb 9, 2016

joleroi added a commit that referenced this pull request Feb 9, 2016

Merge pull request #436 from joleroi/spectrum
Restructure spectrum package and command line tool

@joleroi joleroi merged commit 28fdb6f into gammapy:master Feb 9, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joleroi joleroi deleted the joleroi:spectrum branch Feb 9, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 9, 2016

Member

What about this docs page?
https://gammapy.readthedocs.org/en/latest/spectrum/spectral_fitting.html

Has it been superceded by this one?
https://github.com/gammapy/gammapy/tree/master/docs/tutorials/gammapy-spectrum

Or do you want to keep both?

When I wanted to try out the new spectrum fitting code, https://gammapy.readthedocs.org/en/latest/spectrum/spectral_fitting.html is what I found first,
but it looks like it doesn't quite work any more:

$ gammapy-spectrum crab_config.yaml
Usage: gammapy-spectrum [OPTIONS] COMMAND [ARGS]...

Error: No such command "crab_config.yaml".
$ gammapy-spectrum 
Usage: gammapy-spectrum [OPTIONS] COMMAND [ARGS]...

  Gammapy tool for spectrum extraction and fitting.

  Examples
  --------

  TODO

Options:
  -h, --help  Show this message and exit.

Commands:
  all      Fit spectral model to 1D spectrum
  display  Display results table of one or several...
  extract  Extract 1D spectral information from event...
  fit      Fit spectral model to 1D spectrum
  pipe     Run spectrum analysis pipeline
  plot     Plot spectrum results file
$ gammapy-spectrum all crab_config.yaml
Traceback (most recent call last):
  File "/Users/deil/Library/Python/3.4/bin/gammapy-spectrum", line 9, in <module>
    load_entry_point('gammapy', 'console_scripts', 'gammapy-spectrum')()
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 716, in __call__
    return self.main(*args, **kwargs)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/Users/deil/code/gammapy/gammapy/scripts/spectrum.py", line 67, in all_spectrum
    fit, analysis = run_spectrum_analysis_using_config(config)
  File "/Users/deil/code/gammapy/gammapy/spectrum/spectrum_pipe.py", line 113, in run_spectrum_analysis_using_config
    analysis = run_spectrum_extraction_using_config(config)
  File "/Users/deil/code/gammapy/gammapy/spectrum/spectrum_extraction.py", line 632, in run_spectrum_extraction_using_config
    config = config['extraction']
KeyError: 'extraction'
Member

cdeil commented Feb 9, 2016

What about this docs page?
https://gammapy.readthedocs.org/en/latest/spectrum/spectral_fitting.html

Has it been superceded by this one?
https://github.com/gammapy/gammapy/tree/master/docs/tutorials/gammapy-spectrum

Or do you want to keep both?

When I wanted to try out the new spectrum fitting code, https://gammapy.readthedocs.org/en/latest/spectrum/spectral_fitting.html is what I found first,
but it looks like it doesn't quite work any more:

$ gammapy-spectrum crab_config.yaml
Usage: gammapy-spectrum [OPTIONS] COMMAND [ARGS]...

Error: No such command "crab_config.yaml".
$ gammapy-spectrum 
Usage: gammapy-spectrum [OPTIONS] COMMAND [ARGS]...

  Gammapy tool for spectrum extraction and fitting.

  Examples
  --------

  TODO

Options:
  -h, --help  Show this message and exit.

Commands:
  all      Fit spectral model to 1D spectrum
  display  Display results table of one or several...
  extract  Extract 1D spectral information from event...
  fit      Fit spectral model to 1D spectrum
  pipe     Run spectrum analysis pipeline
  plot     Plot spectrum results file
$ gammapy-spectrum all crab_config.yaml
Traceback (most recent call last):
  File "/Users/deil/Library/Python/3.4/bin/gammapy-spectrum", line 9, in <module>
    load_entry_point('gammapy', 'console_scripts', 'gammapy-spectrum')()
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 716, in __call__
    return self.main(*args, **kwargs)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/Users/deil/code/gammapy/gammapy/scripts/spectrum.py", line 67, in all_spectrum
    fit, analysis = run_spectrum_analysis_using_config(config)
  File "/Users/deil/code/gammapy/gammapy/spectrum/spectrum_pipe.py", line 113, in run_spectrum_analysis_using_config
    analysis = run_spectrum_extraction_using_config(config)
  File "/Users/deil/code/gammapy/gammapy/spectrum/spectrum_extraction.py", line 632, in run_spectrum_extraction_using_config
    config = config['extraction']
KeyError: 'extraction'
@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Feb 9, 2016

Contributor

No this is in fact outdated. I would like to keep it for "low-level" docs. I will remove the broken part in the next PR. Could you try the new tutorial?

Contributor

joleroi commented Feb 9, 2016

No this is in fact outdated. I would like to keep it for "low-level" docs. I will remove the broken part in the next PR. Could you try the new tutorial?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 9, 2016

Member

I'm trying the new tutorial now ... can you come by so that we discuss a bit? ... I think it's easier than via Github for this.

Member

cdeil commented Feb 9, 2016

I'm trying the new tutorial now ... can you come by so that we discuss a bit? ... I think it's easier than via Github for this.

table.add_row(new_row)
l.append(self[key].to_table(**kwargs))
table = vstack(l, join_type='outer')
table.add_column(analyses, index=0)

This comment has been minimized.

@cdeil

cdeil Feb 9, 2016

Member

With Python 3 and Astropy and Gammapy master I'm getting this error when I follow along the new tutorial:

$ gammapy-spectrum display crab_analysis/spectrum_stats.yaml spectrum_stats_23592.yaml
Traceback (most recent call last):
  File "/Users/deil/Library/Python/3.4/bin/gammapy-spectrum", line 9, in <module>
    load_entry_point('gammapy', 'console_scripts', 'gammapy-spectrum')()
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 716, in __call__
    return self.main(*args, **kwargs)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/Users/deil/code/gammapy/gammapy/scripts/spectrum.py", line 99, in display_spectrum
    t = master.to_table(format=format)
  File "/Users/deil/code/gammapy/gammapy/spectrum/results.py", line 762, in to_table
    table.add_column(analyses, index=0)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/astropy/table/table.py", line 1387, in add_column
    self.add_columns([col], [index], rename_duplicate=rename_duplicate)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/astropy/table/table.py", line 1495, in add_columns
    self._init_from_cols(newcols)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/astropy/table/table.py", line 685, in _init_from_cols
    lengths = set(len(col) for col in cols)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/astropy/table/table.py", line 685, in <genexpr>
    lengths = set(len(col) for col in cols)
TypeError: len() of unsized object
@cdeil

cdeil Feb 9, 2016

Member

With Python 3 and Astropy and Gammapy master I'm getting this error when I follow along the new tutorial:

$ gammapy-spectrum display crab_analysis/spectrum_stats.yaml spectrum_stats_23592.yaml
Traceback (most recent call last):
  File "/Users/deil/Library/Python/3.4/bin/gammapy-spectrum", line 9, in <module>
    load_entry_point('gammapy', 'console_scripts', 'gammapy-spectrum')()
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 716, in __call__
    return self.main(*args, **kwargs)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/Users/deil/code/gammapy/gammapy/scripts/spectrum.py", line 99, in display_spectrum
    t = master.to_table(format=format)
  File "/Users/deil/code/gammapy/gammapy/spectrum/results.py", line 762, in to_table
    table.add_column(analyses, index=0)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/astropy/table/table.py", line 1387, in add_column
    self.add_columns([col], [index], rename_duplicate=rename_duplicate)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/astropy/table/table.py", line 1495, in add_columns
    self._init_from_cols(newcols)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/astropy/table/table.py", line 685, in _init_from_cols
    lengths = set(len(col) for col in cols)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/astropy/table/table.py", line 685, in <genexpr>
    lengths = set(len(col) for col in cols)
TypeError: len() of unsized object
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 9, 2016

Member

This is the template render package I mentioned:
http://jinja.pocoo.org/docs/dev/intro/#basic-api-usage

As I said, I'd start by adding the code to make a webpage with the most common plot and numbers for a single analysis, with the plots FitSpectrum generates and prints on the console.
And then, if you want it for your work, expand / improve the subcommands that compare different analyses in text output and plots.
Wanting to fit PL, ECPL and CPL spectral shapes is super common ... this is the one thing where exposing it at the "single analysis config file" level (e.g. as a list in the yaml file) could be useful, or at least it has to be easy / documented how to do this.

Member

cdeil commented Feb 9, 2016

This is the template render package I mentioned:
http://jinja.pocoo.org/docs/dev/intro/#basic-api-usage

As I said, I'd start by adding the code to make a webpage with the most common plot and numbers for a single analysis, with the plots FitSpectrum generates and prints on the console.
And then, if you want it for your work, expand / improve the subcommands that compare different analyses in text output and plots.
Wanting to fit PL, ECPL and CPL spectral shapes is super common ... this is the one thing where exposing it at the "single analysis config file" level (e.g. as a list in the yaml file) could be useful, or at least it has to be easy / documented how to do this.

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