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

Improve significance image analysis #474

Merged
merged 9 commits into from Mar 1, 2016

Conversation

Projects
None yet
3 participants
@adonath
Member

adonath commented Feb 26, 2016

This PR includes a refactoring of the Li&Ma analysis in gammapy. It requires the latest version of gammapy-extra. Docs are still missing, but will be added.

@adonath adonath added this to the 0.4 milestone Feb 29, 2016

@adonath adonath self-assigned this Feb 29, 2016

@adonath adonath force-pushed the adonath:refactor_lima_analysis branch 3 times, most recently from db39568 to 37434e9 Feb 29, 2016

@adonath adonath force-pushed the adonath:refactor_lima_analysis branch from 37434e9 to 6aa6d97 Feb 29, 2016

@adonath

This comment has been minimized.

Member

adonath commented Mar 1, 2016

@cdeil This is ready for further review. I'm currently expanding the docs...but the technical stuff is ready. I'd like to merge this rather soon, beacuse I'm working on a few minor follow up PRs.

def compute_lima_on_off_map(n_on, n_off, a_on, a_off, kernel, exposure=None,
fft=False):

This comment has been minimized.

@cdeil

cdeil Mar 1, 2016

Member

Remove fft option here and in the docstring.

n_off : `~numpy.ndarray`
Off counts map.
a_on : `~numpy.ndarray`
Relative .

This comment has been minimized.

@cdeil

cdeil Mar 1, 2016

Member

The a_on and a_off parameter description is incomplete / incorrect.

excess=n_on_ - background,
alpha=alpha)
if not exposure is None:

This comment has been minimized.

@cdeil

cdeil Mar 1, 2016

Member

if not exposure is None: -> if exposure:

This comment has been minimized.

@adonath

adonath Mar 1, 2016

Member

I just checked, this doesn't work, when exposure is a numpy array. This results in ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

@cdeil

This comment has been minimized.

Member

cdeil commented Mar 1, 2016

As I said recently, I don't think the current API / partitioning of functionality is good.

Putting convolution and flux computation in compute_lima_map makes for a complicated API and means the few key lines to correlate the maps and compute flux will have to be duplicated elsewhere.

But as we discussed right now ... we have bigger fish to fry ... merge and onto a joint fit for two datasets?

class MapsBunch(Bunch):
"""
Minimal version of a future map container class.

This comment has been minimized.

@cdeil

cdeil Mar 1, 2016

Member

Instead of this sentence, put a sentence describing the purpose of this class is now.

Maybe add a code example loading like this to make it easy for people to play around with this?

from gammapy.data import MapsBunch
maps = MapsBunch.read('$GAMMAPY_EXTRA/....something.fits')

Or, if you have that example in the high-level docs, maybe link to the high-level docs page from here?

# safe significance threshold. Is this worse making an option to
# significance()?
significance_lima[n_on_ < 5] = 0

This comment has been minimized.

@cdeil

cdeil Mar 1, 2016

Member

Remove the significance_lima[n_on_ < 5] = 0 call here.
Hard-coding 5 here is bad, the caller has no control.

Note that I very recently added an n_observed_min option here with default value of 1, to get rid of Numpy warnings.
http://gammapy.readthedocs.org/en/latest/api/gammapy.stats.significance.html
So either just remove that option here, or re-expose that option with the same default on this convenience wrapper.

Also ... the formula rendering in that docstring is broken ... could you please fix it?

@@ -510,7 +510,7 @@ def _root_amplitude_brentq(counts, background, model):
amplitude : float
Fitted flux amplitude.
niter : int
Number of function evaluations needed for the fit.
Number of function evaluations` needed for the fit.

This comment has been minimized.

@cdeil

cdeil Mar 1, 2016

Member

I think this backtick got introduced by accident -> remove?

from ...utils.testing import requires_dependency, requires_data
from ...detect import compute_ts_map, compute_lima_map, compute_lima_on_off_map
from ...datasets.core import _GammapyExtra

This comment has been minimized.

@cdeil

cdeil Mar 1, 2016

Member

This is not a good way to access gammapy-extra ... _GammapyExtra is an implementation detail.
You should use

from gammapy.datasets import gammapy_extra
filename = gammapy_extra.filename('logo/gammapy_banner.png')

of, what I like even better because it saves an import:

filename = '$GAMMAPY_EXTRA/logo/gammapy_banner.png'
@adonath

This comment has been minimized.

Member

adonath commented Mar 1, 2016

@cdeil Thanks for the review! As I mentioned before, this doesn't necessarily represent the last final API. It just brings back broken high level functionality and allows to make other existing functionality more uniform and resolve code duplications along with that. A better, more flexible and user-friendly API can easily be later build on top, when we exactly know what it should look like.

@adonath adonath force-pushed the adonath:refactor_lima_analysis branch from 812a33f to b45e51c Mar 1, 2016

cdeil added a commit that referenced this pull request Mar 1, 2016

@cdeil cdeil merged commit 1c9a56c into gammapy:master Mar 1, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@joleroi

This comment has been minimized.

Contributor

joleroi commented Mar 2, 2016

When running my gammapy-spectrum command line tool, I get

[/home/kingj/Software/gammapy-extra/test_datasets/spectrum] gammapy-spectrum extract spectrum_analysis_example.yaml 
Traceback (most recent call last):
  File "/home/kingj/Software/miniconda3/envs/headsherpa/bin/gammapy-spectrum", line 9, in <module>
    load_entry_point('gammapy', 'console_scripts', 'gammapy-spectrum')()
  File "/home/kingj/Software/miniconda3/envs/headsherpa/lib/python2.7/site-packages/setuptools-20.1.1-py2.7.egg/pkg_resources/__init__.py", line 547, in load_entry_point

  File "/home/kingj/Software/miniconda3/envs/headsherpa/lib/python2.7/site-packages/setuptools-20.1.1-py2.7.egg/pkg_resources/__init__.py", line 2720, in load_entry_point

  File "/home/kingj/Software/miniconda3/envs/headsherpa/lib/python2.7/site-packages/setuptools-20.1.1-py2.7.egg/pkg_resources/__init__.py", line 2380, in load

  File "/home/kingj/Software/miniconda3/envs/headsherpa/lib/python2.7/site-packages/setuptools-20.1.1-py2.7.egg/pkg_resources/__init__.py", line 2386, in resolve

  File "/home/kingj/Software/gammapy/gammapy/scripts/__init__.py", line 19, in <module>
    from .image_lima import *
  File "/home/kingj/Software/gammapy/gammapy/scripts/image_lima.py", line 11, in <module>
    from ..detect import compute_lima_map, compute_lima_on_off_map
  File "/home/kingj/Software/gammapy/gammapy/detect/__init__.py", line 7, in <module>
    from .test_statistics import *
  File "/home/kingj/Software/gammapy/gammapy/detect/test_statistics.py", line 22, in <module>
    from ._test_statistics_cython import (_cash_cython, _amplitude_bounds_cython,
ImportError: No module named _test_statistics_cython

This looks like it is related to this PR...

Does anyone have any idea?

@cdeil

This comment has been minimized.

Member

cdeil commented Mar 3, 2016

I don't see this issue.
Can you try make clean?

@joleroi

This comment has been minimized.

Contributor

joleroi commented Mar 3, 2016

Removing the gammapy installation in conda, and rebuilding everything fixed it

@cdeil cdeil changed the title from Refactor lima analysis to Improve significance image analysis Apr 20, 2016

@adonath adonath deleted the adonath:refactor_lima_analysis branch Nov 20, 2018

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