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 EnergyDispersion and CountsSpectrum #330

Merged
merged 10 commits into from Aug 31, 2015

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Aug 18, 2015

Energy Dispersion 2D TODO

  • Fix https://travis-ci.org/gammapy/gammapy/jobs/77662317#L1384
  • Adress Christoph's inline comments
  • The fits file is 40 kB. I'd prefer you gzip it or put it in gammapy-extra to keep the code repo small.
  • Add more tests, especially for I/O (see here for an example).
  • Check EnergyDispersion class (Energy handling, RMF export) -> leave to later
  • More Documentation? -> Later.

@cdeil cdeil added the feature label Aug 18, 2015

@cdeil cdeil added this to the 0.4 milestone Aug 18, 2015

@cdeil cdeil self-assigned this Aug 18, 2015

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Aug 28, 2015

Contributor

Maybe we should focus on the Energy Disperson 2D class in this PR and do the rest of the spectral fitting in a separate one

Contributor

joleroi commented Aug 28, 2015

Maybe we should focus on the Energy Disperson 2D class in this PR and do the rest of the spectral fitting in a separate one

Show outdated Hide outdated gammapy/data/counts_spectrum.py Outdated
Show outdated Hide outdated gammapy/data/counts_spectrum.py Outdated
Show outdated Hide outdated gammapy/data/counts_spectrum.py Outdated
Show outdated Hide outdated gammapy/data/counts_spectrum.py Outdated
class EnergyDispersion2D(object):
"""Offset-dependent energy dispersion matrix.

This comment has been minimized.

@cdeil

cdeil Aug 28, 2015

Member

Document Parameters here?
Move plotting examples to the plot method docstrings?

@cdeil

cdeil Aug 28, 2015

Member

Document Parameters here?
Move plotting examples to the plot method docstrings?

return cls.from_fits(hdulist['ENERGY DISPERSION'])
def evaluate(self, offset=None, e_true=None, migra=None):
"""Probability for a given offset, true energy, and migration

This comment has been minimized.

@cdeil

cdeil Aug 28, 2015

Member

Here you could be more specific and explain that the value is dP/dmigra (if that is the case).
I don't like the name migra but don't have a better suggestion at the moment.

@cdeil

cdeil Aug 28, 2015

Member

Here you could be more specific and explain that the value is dP/dmigra (if that is the case).
I don't like the name migra but don't have a better suggestion at the moment.

This comment has been minimized.

@cdeil

cdeil Aug 28, 2015

Member

Worth adding a Parameters section to this docstring?

@cdeil

cdeil Aug 28, 2015

Member

Worth adding a Parameters section to this docstring?

This comment has been minimized.

@joleroi

joleroi Aug 28, 2015

Contributor

I also don't like the name migra
I guess there should be an explaation of what an RM is and so on in the high level docs. I can do this before the workshop

@joleroi

joleroi Aug 28, 2015

Contributor

I also don't like the name migra
I guess there should be an explaation of what an RM is and so on in the high level docs. I can do this before the workshop

This comment has been minimized.

@cdeil

cdeil Aug 28, 2015

Member

There's the option to use more explicit names like dp_dy, but it's still cryptic, so I don't have a proposal for something better at the moment.

Adding a formula to the docs to better explain what the functions return (i.e. which probability density) is easy.
See here as an example.

@cdeil

cdeil Aug 28, 2015

Member

There's the option to use more explicit names like dp_dy, but it's still cryptic, so I don't have a proposal for something better at the moment.

Adding a formula to the docs to better explain what the functions return (i.e. which probability density) is easy.
See here as an example.

Show outdated Hide outdated gammapy/irf/energy_dispersion.py Outdated
Show outdated Hide outdated gammapy/irf/energy_dispersion.py Outdated
Show outdated Hide outdated gammapy/irf/energy_dispersion.py Outdated
Show outdated Hide outdated gammapy/irf/energy_dispersion.py Outdated
Show outdated Hide outdated gammapy/irf/energy_dispersion.py Outdated
Show outdated Hide outdated gammapy/irf/energy_dispersion.py Outdated
assert_allclose(actual, desired, rtol=1e-06)
# Check that values between node make sense

This comment has been minimized.

@cdeil

cdeil Aug 28, 2015

Member

I'd say let's leave extensive testing to later.
We can use a simple analytical model (e.g. Gauss with some dependence of center and width) and use that for testing the interpolation and density transformation.
But that's at least a day of work, and I'd say let's just run it and see if we get spectral results first.

@cdeil

cdeil Aug 28, 2015

Member

I'd say let's leave extensive testing to later.
We can use a simple analytical model (e.g. Gauss with some dependence of center and width) and use that for testing the interpolation and density transformation.
But that's at least a day of work, and I'd say let's just run it and see if we get spectral results first.

Show outdated Hide outdated gammapy/spectrum/energy.py Outdated
return self[mask]
def select_sky_ring(self, center, inner_radius = None, outer_radius = None,

This comment has been minimized.

@cdeil

cdeil Aug 28, 2015

Member

My suggestion would be to do a simpler API / implementation here:

def select_sky_ring(self, center, inner_radius, outer_radius)

If the user has a thickness, it's still short and readable:

select_sky_ring(center, inner_radius, inner_radius + thickness)

But if you like the flexibility here, OK to keep (the implementation with the many if statements looks correct to me).

@cdeil

cdeil Aug 28, 2015

Member

My suggestion would be to do a simpler API / implementation here:

def select_sky_ring(self, center, inner_radius, outer_radius)

If the user has a thickness, it's still short and readable:

select_sky_ring(center, inner_radius, inner_radius + thickness)

But if you like the flexibility here, OK to keep (the implementation with the many if statements looks correct to me).

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 28, 2015

Member

I've left a few inline comments and updated the task list at the top.
This is looking good already!

Member

cdeil commented Aug 28, 2015

I've left a few inline comments and updated the task list at the top.
This is looking good already!

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Aug 28, 2015

Contributor

Thank you very much, I will focus on getting the fit to work and then clean up after me (maybe after a more thorough discussion about the IRF classes)

Contributor

joleroi commented Aug 28, 2015

Thank you very much, I will focus on getting the fit to work and then clean up after me (maybe after a more thorough discussion about the IRF classes)

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 28, 2015

Member

I'd say let's try to get this in master quickly (so that anyone can start using it) and then if there are changes needed to get the fit to work, do it in follow-up PRs next week.
(this is already a big PR and it gets harder and harder if you want to do more and more stuff in one PR.)

Member

cdeil commented Aug 28, 2015

I'd say let's try to get this in master quickly (so that anyone can start using it) and then if there are changes needed to get the fit to work, do it in follow-up PRs next week.
(this is already a big PR and it gets harder and harder if you want to do more and more stuff in one PR.)

Show outdated Hide outdated gammapy/data/counts_spectrum.py Outdated

@cdeil cdeil changed the title from WIP: Add missing functionality to perform a spectral fit with gammapy.hspec to Add EnergyDispersion and CountsSpectrum Aug 28, 2015

@pytest.mark.xfail
def test_EnergyDispersion():
edisp = EnergyDispersion()
pdf = edisp(3, 4)
assert_allclose(pdf, 42)

This comment has been minimized.

@cdeil

cdeil Aug 28, 2015

Member
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 28, 2015

Member

The np_to_rmf function no longer exists ... this needs to be adapted:
https://travis-ci.org/gammapy/gammapy/jobs/77671805#L1409

Member

cdeil commented Aug 28, 2015

The np_to_rmf function no longer exists ... this needs to be adapted:
https://travis-ci.org/gammapy/gammapy/jobs/77671805#L1409

joleroi pushed a commit that referenced this pull request Aug 31, 2015

Merge pull request #330 from kingj90/spectralfit
Add EnergyDispersion and CountsSpectrum

@joleroi joleroi merged commit b828e03 into gammapy:master Aug 31, 2015

1 check passed

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

joleroi added a commit that referenced this pull request Aug 31, 2015

dlennarz pushed a commit to dlennarz/gammapy that referenced this pull request Oct 11, 2015

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