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

Add EDispMap and PSFMap to MapDataset io #2523

Merged
merged 2 commits into from Nov 8, 2019
Merged

Conversation

@AtreyeeS
Copy link
Member

AtreyeeS commented Nov 7, 2019

This addresses #2513 .
I am not sure what minimal regression test to add without significantly changing test_fit.py

@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 7, 2019

Codecov Report

Merging #2523 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2523      +/-   ##
==========================================
+ Coverage   91.06%   91.08%   +0.01%     
==========================================
  Files         147      147              
  Lines       16861    16871      +10     
==========================================
+ Hits        15355    15367      +12     
+ Misses       1506     1504       -2
Impacted Files Coverage Δ
gammapy/cube/fit.py 89.37% <100%> (+0.52%) ⬆️

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 955a692...f0aa8ce. Read the comment docs.

Copy link
Member

adonath left a comment

Thanks @AtreyeeS, I have left one minor inline comment concerning the hdu name. Otherwise it looks fine to me. However can you still add a small regression test? You can basically create an empty MapDataset (using .create()), write it to disk and read it back. You don't have to assert on the data, just assert, that the .edisp and .psf (and their exposure) is not None.

@@ -633,6 +633,9 @@ def to_hdulist(self):
hdulist.append(hdus["EDISP_MATRIX_EBOUNDS"])
else:
hdulist += self.edisp.edisp_map.to_hdulist(hdu="EDISP")[exclude_primary]
hdulist += self.edisp.exposure_map.to_hdulist(hdu="exp_edisp")[

This comment has been minimized.

Copy link
@adonath

adonath Nov 8, 2019

Member

Minor comment: could you rename the hdu to EDISP_EXPOSURE, just to be more explicit then exp_.

@adonath adonath self-assigned this Nov 8, 2019
@adonath adonath added the bug label Nov 8, 2019
@adonath adonath added this to the 0.15 milestone Nov 8, 2019
@AtreyeeS AtreyeeS requested a review from adonath Nov 8, 2019
@adonath
adonath approved these changes Nov 8, 2019
Copy link
Member

adonath left a comment

Thanks @AtreyeeS. No further comment from my side.

@adonath adonath merged commit 652f1d2 into gammapy:master Nov 8, 2019
11 checks passed
11 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: No new issues – Tests: passed
Details
codecov/patch 100% of diff hit (target 91.06%)
Details
codecov/project 91.08% (+0.01%) compared to 955a692
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20191108.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
@adonath adonath changed the title Modify MapDataset io Add EDispMap and PSFMap to MapDataset io 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.