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 TS map header writing and temp file handling #273

Merged
merged 2 commits into from Aug 13, 2015

Conversation

Projects
None yet
3 participants
@adonath
Member

adonath commented Aug 12, 2015

While running the hgps_survey_map.py example (see here and here, @mapazarr and I noticed that the output HDUs in ts.fits that are written by TSMapResult.write don't contain WCS info in the header (see here).

It's not obvious what the problem is from looking at the code, probably it should start by making a copy of the input header for each HDU and leaving it unmodified.

@adonath @mapazarr I guess each of us could fix this (and add a test that writes - reads - asserts a small example file). If someone has time to do this, please leave a comment here when you start.

@cdeil cdeil added the bug label May 31, 2015

@cdeil cdeil added this to the 0.4 milestone May 31, 2015

@cdeil cdeil referenced this pull request May 31, 2015

Closed

Further TS map improvements #223

6 of 10 tasks complete

@cdeil cdeil modified the milestones: 0.4, 0.3 May 31, 2015

@mapazarr

This comment has been minimized.

Member

mapazarr commented Jun 1, 2015

The problem is not in the TSMapResult.write function. The problem is that the file from which it tries to copy the header has multiple headers. Please see:
gammasky/hess-host-analyses@0f2111a

@mapazarr

This comment has been minimized.

Member

mapazarr commented Jun 1, 2015

I think the problem is that COUNTS_IMAGE gets a header from REF_IMAGE and another one (or more?) from TOTAL_EVENTS_FILE. So when calling 'header = fits.getheader(COUNTS_IMAGE), it doesn't know which one to use, and takes the wrong one. Please note also the following warning, happening while readingTOTAL_EVENTS_FILEto createCOUNTS_IMAGEin the call tomake_background_image(): WARNING: hdu= was not specified but multiple tables are present, reading in first available table (hdu=1) [astropy.io.fits.connect]`

@cdeil

This comment has been minimized.

Member

cdeil commented Aug 2, 2015

@adonath – Can you fix this issue? Move to 0.4 milestone?

@cdeil cdeil modified the milestones: 0.4, 0.3 Aug 3, 2015

@adonath adonath force-pushed the adonath:fix_#273 branch from 31cadab to 57f315d Aug 12, 2015

@adonath

This comment has been minimized.

Member

adonath commented Aug 12, 2015

I couldn't reproduce this bug locally, but now it makes a copy of the header before modifying it. I also added a regression test for the TSMapResult.write() and TSMapResult.read() methods.

@cdeil cdeil modified the milestones: 0.3, 0.4 Aug 12, 2015

@cdeil

This comment has been minimized.

Member

cdeil commented Aug 12, 2015

@adonath - Thanks!

This fixes the issue for me and travis-ci tests pass.

Please add a changelog entry (under the 0.3 section, 😄 )

The new source.fits.gz is filesize 147K, which is borderline for the code repo.
Would you mind moving all poisson_stats_image files to gammapy-extra and adding the remote_data decorator for now.
That's the solution I want to implement soon: all test and example files in the gammapy-extra repo and then include that as a git submodule.
Alternatively, storing that image as float32 instead of float64 would also be an improvement.

I have a few small questions / comments that I'll put inline now.

clobber=True)
fits.PrimaryHDU(data=background(x, y), header=header).writeto('background.fits.gz',
clobber=True)
fits.PrimaryHDU(data=source(x, y), header=header).writeto('source.fits.gz',

This comment has been minimized.

@cdeil

cdeil Aug 12, 2015

Member

This is more readable if you split it in two lines IMO:

...
hdu = fits.PrimaryHDU(data=data, header=header)
hdu.writeto('source.fits.gz', clobber=True)
...
@@ -38,3 +39,11 @@ def test_compute_ts_map():
assert_allclose([[99], [99]], np.where(result.ts == result.ts.max()))
assert_allclose(6, result.niter[99, 99])
assert_allclose(1.0227934338735763e-09, result.amplitude[99, 99], rtol=1e-3)
# test write method
with NamedTemporaryFile(suffix='.fits') as f:

This comment has been minimized.

@cdeil

cdeil Aug 12, 2015

Member

This is not how I've used NamedTemporaryFile in other places:
https://github.com/gammapy/gammapy/search?utf8=%E2%9C%93&q=namedtemporaryfile

I don't know how to use it properly, so this is a question: which is better?

The drawback of how you use it is that all code is indented one level more ... if possible I'd like to avoid this.
With the way I use it, it's not clear to me if that is "safe", i.e. when Python opens or closes this temp file, and why I can open it a second time (?) by using the file name.

@adonath – If you know what's going on here, or have time to find out, making this uniform in Gammapy and adding a very short entry to the dev doc HOWTO page would be very helpful.

This comment has been minimized.

@adonath

adonath Aug 13, 2015

Member

I think the recommended way to use file objects is the with context. I don't know about temporary files, but for 'normal' files this ensures, that the file is closed again, even if the code fails in between. And f.close(), which does the garbage collection and maybe deletes the temporary file, doesn't have to be called explicitly. In some places in Gammapy, where NamedTemporaryFile objects are used, they aren't closed again, which I think is bad style and could lead to unexpected errors.

@cdeil

This comment has been minimized.

Member

cdeil commented Aug 12, 2015

Otherwise this looks good ... feel free to merge once the comments are addressed.

@adonath adonath force-pushed the adonath:fix_#273 branch 2 times, most recently from 70e66b8 to 58061af Aug 13, 2015

@adonath adonath force-pushed the adonath:fix_#273 branch from 58061af to b8d6176 Aug 13, 2015

cdeil added a commit that referenced this pull request Aug 13, 2015

Merge pull request #273 from adonath/fix_#273
TSMapResult.write header argument doesn't work

@cdeil cdeil merged commit b22a3ad into gammapy:master Aug 13, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@cdeil cdeil changed the title from TSMapResult.write header argument doesn't work to Fix writing TS map headers Sep 16, 2015

@cdeil cdeil changed the title from Fix writing TS map headers to Fix TS map header writing and temp file handling Sep 16, 2015

@adonath adonath deleted the adonath:fix_#273 branch Nov 20, 2018

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