Skip to content
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

Remove unused functions from gammapy.irf #2552

Merged
merged 6 commits into from Nov 16, 2019

Conversation

@adonath
Copy link
Member

adonath commented Nov 16, 2019

Description
This PR removes some unused functions from gammapy.irf and gammapy.cube:

  • Remove untested and unused _fov_background_norm. The plan is to introduce this as a FoVBackgroundMaker or similar
  • Remove apply_containment_fraction() and implement the 2 lines of code, where they are used
  • Remove compute_energy_thresholds(), which is now superseded by the SafeMaskMaker
  • Remove make_mean_edisp(), the same can be achieved with SpectrumDataset.stack() now.

Dear reviewer
No real review required, once the CI builds are green I'll go ahead an merge.

@adonath adonath self-assigned this Nov 16, 2019
@adonath adonath added this to the 0.15 milestone Nov 16, 2019
@adonath adonath added the cleanup label Nov 16, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 16, 2019

Codecov Report

Merging #2552 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2552      +/-   ##
==========================================
+ Coverage   91.26%   91.31%   +0.04%     
==========================================
  Files         144      144              
  Lines       16282    16213      -69     
==========================================
- Hits        14860    14805      -55     
+ Misses       1422     1408      -14
Impacted Files Coverage Δ
gammapy/cube/background.py 100% <ø> (+20.58%) ⬆️
gammapy/irf/irf_reduce.py 100% <ø> (+9.45%) ⬆️
gammapy/spectrum/make.py 97.05% <100%> (ø) ⬆️
gammapy/spectrum/dataset.py 95.07% <100%> (+0.06%) ⬆️
gammapy/cube/fit.py 89.66% <100%> (-0.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a28028c...244107c. Read the comment docs.

@adonath adonath merged commit 918efd8 into gammapy:master Nov 16, 2019
11 of 12 checks passed
11 of 12 checks passed
greeting
Details
Scrutinizer Errored
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch 100% of diff hit (target 91.26%)
Details
codecov/project 91.31% (+0.04%) compared to a28028c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20191116.7 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Nov 17, 2019

@adonath - What about make_mean_psf? (see here)

Do you want to keep it or replace it with something else?

I noticed it in static analysis, because PyCharm complains about the coding pattern that can lead to UnboundLocalError:

>>> from gammapy.irf import make_mean_psf
>>> make_mean_psf([], 42)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/deil/work/code/gammapy/gammapy/irf/irf_reduce.py", line 81, in make_mean_psf
    return stacked_psf
UnboundLocalError: local variable 'stacked_psf' referenced before assignment

I guess if the code to create one object isn't a lot, then we should put one line code duplication before and in the loop, and rewrite it like this?

def make_mean_psf(observations, position, energy=None, rad=None):
    psf = make_psf(observations[0], position, energy, rad)
    for observation in observations[1:]:
        psf_obs = make_psf(observation, position, energy, rad)
        psf = psf.stack(psf_obs)

    return psf

This will lead to an IndexError, which seems appropriate for that case, and is much nicer for users than an UnboundLocalError.

Another coding pattern we could use:

def make_mean_psf(observations, position, energy=None, rad=None):
    return EnergyDependentTablePSF.from_stack(
        make_psf(obs, position, energy, rad)
        for obs in observations
    )

This might be a general point for functions where we accumulate or stack, i.e. we should probably clean up in other places as well and always use the same coding pattern.

@adonath adonath changed the title Remove unused functions from `gammapy.irf` Remove unused functions from gammapy.irf Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.