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

Fix `frame` attribute of SkyDiffuseCube and SkyDiffuseMap #2108

Merged
merged 2 commits into from Apr 3, 2019

Conversation

Projects
None yet
2 participants
@lmohrmann
Copy link
Contributor

commented Apr 2, 2019

After having added the frame attribute to SkyDiffuseMap in #2106, I noticed that currently this attribute returns an astropy frame object, whereas all other models just give the name of the frame (e.g. icrs). Indeed, a string is expected e.g. here.

This PR changes the frame attribute of SkyDiffuseMap and SkyDiffuseCube such that the name of the frame is returned.

I wanted to modify the test for the SkyDiffuseCube as well, but could not run the test since $GAMMAPY_DATA/tests/unbundled/fermi/gll_iem_v02_cutout.fits could not be found (and is not retrieved by gammapy download tutorials --release 0.11). Help is appreciated here.

@lmohrmann lmohrmann requested a review from adonath Apr 2, 2019

@lmohrmann

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

I added another commit that fixes (all but one of) the failing tests.

I think the tests run by the TestSkyDiffuseCubeMapEvaluator were "accidentally working" previously. They should have failed, since the diffuse_model was created with coordsys="CEL" (by default), but geom was created with coordsys="GAL". In the diffuse_evaluator, this should have clashed, but because the frame attribute of the diffuse_model was an astropy frame object (and not the string "icrs"), this line evaluated to "GAL" rather than "CEL" - and the tests worked. When I changed the frame attribute to return a string, the tests failed (indeed as expected). They pass again when the diffuse_model is created with coordsys="GAL" as well.

@adonath adonath self-assigned this Apr 2, 2019

@adonath adonath added feature bug and removed feature labels Apr 2, 2019

@adonath adonath added this to the 0.12 milestone Apr 2, 2019

@adonath

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Thanks for this fix @lmohrmann! I expected the statement comparing the Astropy frame object to a string to evaluate correctly, but you're right it does not.

@adonath adonath merged commit ac397fd into gammapy:master Apr 3, 2019

9 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 2 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20190402.4 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python35) Test Python35 succeeded
Details
gammapy.gammapy (Test Windows35) Test Windows35 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details

@lmohrmann lmohrmann deleted the lmohrmann:fix-diffuse-model-frame branch Apr 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.