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 flux point estimation for multiple observations #1316

Merged
merged 2 commits into from Feb 20, 2018

Conversation

Projects
None yet
2 participants
@lmohrmann
Contributor

lmohrmann commented Feb 16, 2018

These are some small fixes to make it possible to run the FluxPointEstimator on a SpectrumObservationList (rather than a single SpectrumObservation).

The most important point is that the computation of upper limits needs to be fixed, since that relied on there being only a single entry in the result attribute of the fit.

The changes in gammapy/spectrum/fit.py are necessary to catch cases in which one of the observations in the list has no "safe" bins within the energy bounds specified by the FluxPointEstimator.

@lmohrmann lmohrmann requested review from adonath and joleroi Feb 16, 2018

@lmohrmann

This comment has been minimized.

Contributor

lmohrmann commented Feb 20, 2018

@adonath @joleroi Has either of you time to have a quick look at this?

If not, @cdeil, could you have a look or suggest another reviewer?

Thanks!

@cdeil cdeil added the feature label Feb 20, 2018

@cdeil cdeil added this to the 0.7 milestone Feb 20, 2018

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 20, 2018

@lmohrmann - Can you please adapt the tests to make this pass?

python -m pytest -v gammapy/spectrum/tests/test_flux_point.py

https://gist.github.com/cdeil/ff27083f3afa01de418c6fef1d49f1ec

We plan to heavily work on 1D spectral analysis, but I'd suggest we just merge this in as soon as CI passes and then continue on Monday from master.

@lmohrmann

This comment has been minimized.

Contributor

lmohrmann commented Feb 20, 2018

@cdeil I think the failing tests were due to a small bug I introduced. Could you check again? I tried to run the tests using the command you pasted above, but this fails like so:

INTERNALERROR>   File "/home/hpc/caph/mpp227/software/gammapy/gammapy/maps/sparse.py", line 4, in <module>
INTERNALERROR>     from ._sparse import find_in_array, merge_sparse_arrays
INTERNALERROR> ImportError: No module named _sparse

The CI fails are unrelated, I think.

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 20, 2018

@lmohrmann - If you use python -m pytest, you're running from the source folder. So you have to do an in-place build, using pip install -e . (this is running python setup.py develop, which is running python setup.py build_ext -i, so those are really equivalent if you prefer to use setup.py instead of pip)

The python setup.py test builds and copies everything to a temp folder for each test invocation, so the issue doesn't appear there. But that is slower, so I prefer python -m pytest and pip install -e ..

@cdeil cdeil merged commit d6d2310 into gammapy:master Feb 20, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@lmohrmann lmohrmann deleted the lmohrmann:flux-points-on-obs-list branch Feb 20, 2018

@cdeil cdeil changed the title from Make the FluxPointEstimator work on multiple observations to Add flux point estimation for multiple observations Feb 28, 2018

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