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 MapMakerRing #2525

Merged
merged 2 commits into from Nov 10, 2019
Merged

Remove MapMakerRing #2525

merged 2 commits into from Nov 10, 2019

Conversation

@luca-giunti
Copy link
Contributor

luca-giunti commented Nov 9, 2019

This PR dissolves the MapMakerRing class. The new pattern for the ring background estimation will now be the following:

stacked = MapDatasetOnOff.create(...)
ring_bkg_maker = RingBackgroundMaker(...)
map_dataset_maker = MapDatasetMaker(...)

for obs in observations:
    dataset = map_dataset_maker.run(obs)
    dataset = dataset.to_image()
    
    dataset_on_off = ring_bkg_maker.run(dataset)
    stacked.stack(dataset_on_off)
@luca-giunti luca-giunti requested a review from adonath Nov 9, 2019
@cdeil cdeil changed the title Removed-RingBackgroundMaker Remove MapMakerRing Nov 9, 2019
@cdeil cdeil added the cleanup label Nov 9, 2019
@cdeil cdeil added this to the 0.15 milestone Nov 9, 2019
@cdeil cdeil added this to In progress in gammapy.cube via automation Nov 9, 2019
@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Nov 9, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 9, 2019

Codecov Report

Merging #2525 into master will increase coverage by 0.25%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2525      +/-   ##
==========================================
+ Coverage   91.23%   91.49%   +0.25%     
==========================================
  Files         146      146              
  Lines       16792    16729      -63     
==========================================
- Hits        15321    15306      -15     
+ Misses       1471     1423      -48
Impacted Files Coverage Δ
gammapy/cube/make.py 97.76% <100%> (+24.6%) ⬆️
gammapy/modeling/parameter.py 94.94% <0%> (-0.23%) ⬇️
gammapy/modeling/models/spatial.py 99.13% <0%> (ø) ⬆️
gammapy/modeling/models/cube.py 94.28% <0%> (+0.09%) ⬆️

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 b72365d...afa3bc2. Read the comment docs.

Copy link
Member

cdeil left a comment

@luca-giunti - Thanks!

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Nov 10, 2019

@luca-giunti - I'm not sure what you did in 24f2f0d - tutorials/image_analysis.ipynb is not a valid IPYNB JSON file any more.
Did you edit the JSON with a text editor?

https://dev.azure.com/gammapy/gammapy/_build/results?buildId=2350&view=logs&jobId=562d890d-bc3a-5a9d-5926-900cbb5c7a53&taskId=3d609307-b051-501f-1b3a-7dd0e405b3a3&lineStart=149&lineEnd=150&colStart=1&colEnd=1

I'll remove that commit here now and merge this PR in.

@cdeil cdeil assigned cdeil and unassigned adonath Nov 10, 2019
@cdeil cdeil dismissed their stale review via 9ddd844 Nov 10, 2019
@cdeil cdeil force-pushed the luca-giunti:Remove-MapMakerRing branch from 24f2f0d to 9ddd844 Nov 10, 2019
@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Nov 10, 2019

I removed the reference in the notebook and fixed two docstrings in afa3bc2 .

@cdeil
cdeil approved these changes Nov 10, 2019
@cdeil cdeil merged commit a244346 into gammapy:master Nov 10, 2019
6 of 9 checks passed
6 of 9 checks passed
Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
Scrutinizer Running
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
gammapy.gammapy Build #20191110.5 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
gammapy.cube automation moved this from In progress to Done Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.cube
  
Done
3 participants
You can’t perform that action at this time.