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

Rewrite spectrum analysis #396

Merged
merged 26 commits into from Dec 11, 2015

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Dec 7, 2015

This PR (hopefully) brings the Spectrum Analysis to a state where other people can use it.

  • Option to use plain sherpa instead of hspec
  • Split out fitting into separate class
  • Document
  • Add tests

@joleroi joleroi self-assigned this Dec 7, 2015

@joleroi joleroi added the feature label Dec 7, 2015

@joleroi joleroi added this to the 0.5 milestone Dec 7, 2015

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Dec 10, 2015

Contributor

In principle this is finished (except some minor doc fixes and probably test failures). If someone wants to have a look, please do so. Otherwise I'll merge in the next days.

Contributor

joleroi commented Dec 10, 2015

In principle this is finished (except some minor doc fixes and probably test failures). If someone wants to have a look, please do so. Otherwise I'll merge in the next days.

@cdeil cdeil modified the milestones: 0.4, 0.5 Dec 10, 2015

@cdeil cdeil referenced this pull request Dec 10, 2015

Merged

Use external ci-helpers #392

Show outdated Hide outdated docs/spectrum/index.rst Outdated
Show outdated Hide outdated docs/spectrum/index.rst Outdated
Show outdated Hide outdated gammapy/obs/data_store.py Outdated
Show outdated Hide outdated gammapy/spectrum/spectrum_analysis.py Outdated
Show outdated Hide outdated gammapy/spectrum/spectrum_analysis.py Outdated
return fit
def get_containment(self):
"""Calculate PSF correction factor for containment in ON region"""

This comment has been minimized.

@cdeil

cdeil Dec 10, 2015

Member

Leave a TODO comment here?

@cdeil

cdeil Dec 10, 2015

Member

Leave a TODO comment here?

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

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 10, 2015

Member

I've left a few comments. My preference would be you merge this tomorrow ... it's more realistic / simple that people start using this if it's in master.

The travis-ci fails are unrelated, see comment here:
#392 (comment)
Feel free to merge this PR even if those fails are still present.

Member

cdeil commented Dec 10, 2015

I've left a few comments. My preference would be you merge this tomorrow ... it's more realistic / simple that people start using this if it's in master.

The travis-ci fails are unrelated, see comment here:
#392 (comment)
Feel free to merge this PR even if those fails are still present.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 11, 2015

Member

@joleroi - If you have time:

Please delete https://github.com/gammapy/gammapy/tree/master/docs/tutorials/gammapy-pfspec and replace it with some equivalent high-level docs for the new functionality.
A picture of the fitted spectrum and the actual reflected regions used for that Crab analysis would be great ... we can extend it with flux points and butterfly later.

This will probably be the most-used feature in Gammapy for the coming months, so making a nice docs page for this and linking to it first thing from the 0.4 release notes is important.

Member

cdeil commented Dec 11, 2015

@joleroi - If you have time:

Please delete https://github.com/gammapy/gammapy/tree/master/docs/tutorials/gammapy-pfspec and replace it with some equivalent high-level docs for the new functionality.
A picture of the fitted spectrum and the actual reflected regions used for that Crab analysis would be great ... we can extend it with flux points and butterfly later.

This will probably be the most-used feature in Gammapy for the coming months, so making a nice docs page for this and linking to it first thing from the 0.4 release notes is important.

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Dec 11, 2015

Contributor

Have you seen https://github.com/joleroi/gammapy/blob/spectrum_analysis/docs/spectrum/index.rst ?
If yes: what would you like to add?

Contributor

joleroi commented Dec 11, 2015

Have you seen https://github.com/joleroi/gammapy/blob/spectrum_analysis/docs/spectrum/index.rst ?
If yes: what would you like to add?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 11, 2015

Member

No, I haven't read through that, sorry.

At a quick glance, it's not what I think is best to introduce / advertise this to end users.

What I think would be great would be something like http://cxc.harvard.edu/sherpa/threads/pha_intro/, i.e. a worked example with analysis result printout and one or two nice pictures.

Once that's in place, maybe we can even remove
https://hess-confluence.desy.de/confluence/display/HESS/HESS+Open+Source+Tools+-+HOWTO+HESS+spectral+analysis+with+Gammapy to reduce docs maintenance effort?

It's not quite clear to me: this tutorial analysis shouldn't run during sphinx_build, right?
If so, the PNG images have to be stored in the repo in the docs folder.
Maybe if you make them rather small (~100 kB each or less), that's OK?
Or maybe put the images in gammapy-extra and get them during the sphinx build somehow ... I think the Sphinx build already depends on gammapy-extra being available.

Member

cdeil commented Dec 11, 2015

No, I haven't read through that, sorry.

At a quick glance, it's not what I think is best to introduce / advertise this to end users.

What I think would be great would be something like http://cxc.harvard.edu/sherpa/threads/pha_intro/, i.e. a worked example with analysis result printout and one or two nice pictures.

Once that's in place, maybe we can even remove
https://hess-confluence.desy.de/confluence/display/HESS/HESS+Open+Source+Tools+-+HOWTO+HESS+spectral+analysis+with+Gammapy to reduce docs maintenance effort?

It's not quite clear to me: this tutorial analysis shouldn't run during sphinx_build, right?
If so, the PNG images have to be stored in the repo in the docs folder.
Maybe if you make them rather small (~100 kB each or less), that's OK?
Or maybe put the images in gammapy-extra and get them during the sphinx build somehow ... I think the Sphinx build already depends on gammapy-extra being available.

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Dec 11, 2015

Contributor

Do you agree to leave this high-level tutorial for a new PR? It need some work in terms of visualization and I would prefer to get this stuff in master so Lea et al. can start using it

Contributor

joleroi commented Dec 11, 2015

Do you agree to leave this high-level tutorial for a new PR? It need some work in terms of visualization and I would prefer to get this stuff in master so Lea et al. can start using it

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 11, 2015

Member

Do you agree to leave this high-level tutorial for a new PR?

Yes!

Concerning https://github.com/gammapy/gammapy/tree/master/docs/tutorials/gammapy-pfspec , I think it can be deleted now. Although, if you think it's a useful reference to have in master when writing the new tutorial, OK to keep it a bit longer.

I'm offline now until ~ 8 pm ... feel free to merge this yourself, or I will tonight.

Member

cdeil commented Dec 11, 2015

Do you agree to leave this high-level tutorial for a new PR?

Yes!

Concerning https://github.com/gammapy/gammapy/tree/master/docs/tutorials/gammapy-pfspec , I think it can be deleted now. Although, if you think it's a useful reference to have in master when writing the new tutorial, OK to keep it a bit longer.

I'm offline now until ~ 8 pm ... feel free to merge this yourself, or I will tonight.

@joleroi joleroi modified the milestones: 0.5, 0.4 Dec 11, 2015

@joleroi joleroi changed the title from Spectrum analysis to Rewrite spectrum analysis Dec 11, 2015

joleroi added a commit that referenced this pull request Dec 11, 2015

@joleroi joleroi merged commit fcbf708 into gammapy:master Dec 11, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@joleroi joleroi deleted the joleroi:spectrum_analysis branch Dec 11, 2015

result = old.copy()
for k, v in new.iteritems():
if k in result and isinstance(result[k], dict):
result[k] = recursive_update(result[k], v)

This comment has been minimized.

@cdeil

cdeil Dec 13, 2015

Member

This is a bug ... it should be recursive_update_dict, not recursive_update.

@cdeil

cdeil Dec 13, 2015

Member

This is a bug ... it should be recursive_update_dict, not recursive_update.

The built-in update function cannot be used for hierarchical dicts
where only a subset of keys shall be updated.
see: http://stackoverflow.com/questions/3232943/update-value-of-a-nested-dictionary-of-varying-depth

This comment has been minimized.

@cdeil

cdeil Dec 13, 2015

Member

Maybe take this code and link to it directly?
http://stackoverflow.com/a/3233356

@cdeil

cdeil Dec 13, 2015

Member

Maybe take this code and link to it directly?
http://stackoverflow.com/a/3233356

return Path(expandvars(str(path)))
def recursive_update_dict(old, new):

This comment has been minimized.

@cdeil

cdeil Dec 13, 2015

Member

Update suggests in-place modification, like dict.update.
You call copy, i.e. the inputs are unmodified, right?

In this case I'd suggest calling the function merge_dict instead and also to explain in the docstring better that a new merged dict is returned and the inputs are unmodified.
(Example: https://github.com/ImmobilienScout24/yamlreader/blob/master/src/main/python/yamlreader/yamlreader.py#L24)

Actually: does this function even work properly? You call it recursively and it calls copy each time. I'm not saying it doesn't work, it's just not obvious to me.

Adding an Example section with one or two examples to the docstring and putting those as unit tests would be very helpful.

Although ... we need a more full-featured solution for config handling instead of building such utilities piece by piece.
I'll make an issue to discuss options shortly and then we can switch to that for 0.5. So @joleroi, please fix the bug at least for 0.4, but don't spend too much time on this.

@cdeil

cdeil Dec 13, 2015

Member

Update suggests in-place modification, like dict.update.
You call copy, i.e. the inputs are unmodified, right?

In this case I'd suggest calling the function merge_dict instead and also to explain in the docstring better that a new merged dict is returned and the inputs are unmodified.
(Example: https://github.com/ImmobilienScout24/yamlreader/blob/master/src/main/python/yamlreader/yamlreader.py#L24)

Actually: does this function even work properly? You call it recursively and it calls copy each time. I'm not saying it doesn't work, it's just not obvious to me.

Adding an Example section with one or two examples to the docstring and putting those as unit tests would be very helpful.

Although ... we need a more full-featured solution for config handling instead of building such utilities piece by piece.
I'll make an issue to discuss options shortly and then we can switch to that for 0.5. So @joleroi, please fix the bug at least for 0.4, but don't spend too much time on this.

def make_path(path):
"""
Expand environment varibles on `~pathlib.Path` construction

This comment has been minimized.

@cdeil

cdeil Dec 13, 2015

Member

Typoe: varibles -> variables.

PS: a good Python editor (like PyCharm out of the box, or Emacs configured well) will show you typos and bugs like the mistyped recursive_update directly. :-)

@cdeil

cdeil Dec 13, 2015

Member

Typoe: varibles -> variables.

PS: a good Python editor (like PyCharm out of the box, or Emacs configured well) will show you typos and bugs like the mistyped recursive_update directly. :-)

"""
result = old.copy()
for k, v in new.iteritems():

This comment has been minimized.

@cdeil

cdeil Dec 13, 2015

Member

.iteritems doesn't exist in Python 3 ... use .items to make it work in Py 2 / 3.

Maybe use this example to add a test to make sure it works on Python 2 / 3?

>>> first = dict(a=42, b=dict(c=43, e=44))
>>> second = dict(d=99, b=dict(g=98, c=50))
>>> from gammapy.utils.scripts import recursive_update_dict
>>> recursive_update_dict(first, second)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/deil/code/gammapy/gammapy/utils/scripts.py", line 204, in recursive_update_dict
    for k, v in new.iteritems():
AttributeError: 'dict' object has no attribute 'iteritems'
@cdeil

cdeil Dec 13, 2015

Member

.iteritems doesn't exist in Python 3 ... use .items to make it work in Py 2 / 3.

Maybe use this example to add a test to make sure it works on Python 2 / 3?

>>> first = dict(a=42, b=dict(c=43, e=44))
>>> second = dict(d=99, b=dict(g=98, c=50))
>>> from gammapy.utils.scripts import recursive_update_dict
>>> recursive_update_dict(first, second)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/deil/code/gammapy/gammapy/utils/scripts.py", line 204, in recursive_update_dict
    for k, v in new.iteritems():
AttributeError: 'dict' object has no attribute 'iteritems'

@cdeil cdeil modified the milestones: 0.4, 0.5 May 19, 2016

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