#102 Add multi-image FITS I/O functions #144

Merged
merged 11 commits into from May 11, 2012

Conversation

Projects
None yet
4 participants
Owner

rmjarvis commented May 10, 2012

Four new functions and some mild clean up of the previous FITS I/O functions:

  • These read/write multi-extension fits files with each image in a separate hdu:
readMulti(fits)
writeMulti(image_list, fits, add_wcs=True, clobber=True)
  • These read/write fits data cubes. i.e. a 3-d array all in one hdu:
readCube(fits)
writeCube(image_list, fits, add_wcs=True, clobber=True):
  • Removed the confusing read as a static method of Image and ImageView classes. This was confusing because it doesn't necessarily return the type specified. Rather the data type is determined from the fits file being read, not the class being used to call the function. Everyone who used the function used the galsim.fits.read syntax, so it was pretty painless to remove.
  • Added the CD matrix to the WCS being written to the fits headers. Considering that this was extolled as an advantage to keeping the pixel size in the Image classes, it seemed like we should actually implement it.
  • Added unit tests for the write function as well as all the new ones. (Only read had been unit tested, comparing the read values to a reference image.)
Owner

rmjarvis commented May 10, 2012

Not sure why it putting the read and write functions on the same line. Obviously those are two different functions in each case.

Owner

barnabytprowe commented May 10, 2012

Hi Mike - am I right in interpreting the changes here that the external FITS files used for testing have been removed, and instead the FITS images are internally generated?

If so, I have concerns that this damages the generality of the unit tests. Those FITS images were made externally (IDL) and this fact alone was enough to catch an important issue (the little-endian / big-endian stuff, resulting in the case catching you see in fits.py). There may be more we haven't come across yet - I don't know, but I'm not prepared to bet against it.

So, if anything, I'd prefer to see more independently-generated FITS test images in the repo for these tests, from alternative sources/systems, rather than fewer. They are tiny files.

Owner

rmjarvis commented May 10, 2012

That's fine. The problem with the old ones was they were square. So that misses any errors in the nx vs ny, which are easy to make. I had one in my readCube function, for example. So if you want to make fits files from IDL that match these, I'm fine with adding that unit test back in.

Owner

rmjarvis commented May 10, 2012

I guess I should mention that I don't have IDL nor even know how to use it. Otherwise I'd do that myself.

Owner

rmandelb commented May 10, 2012

I agree with Barney's concern about unit tests. But otherwise, it looks good to me.

Owner

barnabytprowe commented May 10, 2012

OK Mike, fair enough, I'll make fits files you need and push them.

Owner

barnabytprowe commented May 10, 2012

Hi all, I've put the new IDL-generated FITS files into the repo.

I don't want to put the IDL code used to make them into the repo, not least as it has other dependencies (IDL Astrolib). So, just in case I want to do this again, I'm going to take the liberty of repeating it here for posterity and in all it's clunky IDL glory...

PRO make_test_ims

; Single images
test_array = [[11, 21, 31, 41, 51, 61, 71], $
              [12, 22, 32, 42, 52, 62, 72], $
              [13, 23, 33, 43, 53, 63, 73], $
              [14, 24, 34, 44, 54, 64, 74], $ 
              [15, 25, 35, 45, 55, 65, 75]]
writefits, 'testS.fits', test_array
writefits, 'testI.fits', long(test_array)
writefits, 'testF.fits', float(test_array)
writefits, 'testD.fits', double(test_array)

; Then do cubes with NIMAGES = 12 and multi-extension FITS images with
; each extension
; having ext_no added to it
nimages = 12
test_cube = intarr([size(test_array, /DIM), 12])
for k=0, nimages-1 do begin

   test_cube[*, *, k] = test_array + k

endfor

; First write these cubes out
writefits, 'test_cubeS.fits', test_cube
writefits, 'test_cubeI.fits', long(test_cube)
writefits, 'test_cubeF.fits', float(test_cube)
writefits, 'test_cubeD.fits', double(test_cube)

; Multi-ext: start with 16 bit ints
filename = 'test_multiS.fits'
spawn, "rm "+filename
mkhdr, header, test_array, /EXTEND
writefits, filename, test_array, header
for k=1, nimages-1 do begin

   writefits, filename, test_cube[*, *, k], /APPEND

endfor
; then proceed to other types
filename = 'test_multiI.fits'
spawn, "rm "+filename
mkhdr, header, long(test_array), /EXTEND
writefits, filename, long(test_array), header
for k=1, nimages-1 do begin

   writefits, filename, long(test_cube[*, *, k]), /APPEND

endfor
;
filename = 'test_multiF.fits'
spawn, "rm "+filename
mkhdr, header, float(test_array), /EXTEND
writefits, filename, float(test_array), header
for k=1, nimages-1 do begin

   writefits, filename, float(test_cube[*, *, k]), /APPEND

endfor
;
filename = 'test_multiD.fits'
spawn, "rm "+filename
mkhdr, header, double(test_array), /EXTEND
writefits, filename, double(test_array), header
for k=1, nimages-1 do begin

   writefits, filename, double(test_cube[*, *, k]), /APPEND

endfor
END

Right, I'm very happy to merge this. There was one thing I noted: that readMulti crashes when reading in multiple extension fits where the primary HDU is empty (i.e. if all the data has gone into the extensions). I'm not sure if we ever expect data like that, but I do know that I inadvertently made some for the tests and it took a little bit of hamfisted IDL to work.

If you guys think this is worth it, I'll open an issue to think about this and maybe address it.

Owner

rmjarvis commented May 10, 2012

I think I would put this in the repo if you think it will be useful to make similar things down the line. It doesn't have to be runnable from galsim prerequisites. I would even call it a .txt file to make it clear this isn't meant to be runnable code. Just a guide for running something like it in the future.

Owner

rmjarvis commented May 10, 2012

As for the empty primary HDU thing, I don't mind adding that as a feature. We can check the first one, and if empty then skip it. (Is it testable as if not hdu in python? Or is it more complicated, like there is an hdu but the data array comes back as zero size?

Owner

rmandelb commented May 10, 2012

A few comments:

  1. everything compiles, tests pass, etc. on my machine
  2. I'm fine with merging
  3. I agree with Mike: we could even make a directory for these extra utilities... e.g., I have an IDL script from this morning that I used to make the postage stamps and catalogs that it would be good to have here on github. I'm sure that when we start doing validation / comparison with imengine and the GREAT08 and GREAT10 codes, we will have scripts and things that have other dependencies that we might want to save as well.
Owner

rmjarvis commented May 10, 2012

I just looked at the diff for your commit. I don't like removing the tests of the write functions. These need to be unit tested as well. I thought you were just going to add unit tests with the IDL files as well. The write functions should write to a different name and check that reading the image back in works as expected.

Owner

barnabytprowe commented May 10, 2012

Ah that was a mistake, apologies, I'm flagging. Right, I'll try to fix it.

On 10 May 2012, at 14:29, Mike Jarvis wrote:

I just looked at the diff for your commit. I don't like removing the tests of the write functions. These need to be unit tested as well. I thought you were just going to add unit tests with the IDL files as well. The write functions should write to a different name and check that reading the image back in works as expected.


Reply to this email directly or view it on GitHub:
#144 (comment)

Barnaby Rowe

Jet Propulsion Laboratory
California Institute of Technology
MS 169-237
4800 Oak Grove Drive
Pasadena CA 91109
United States of America

Department of Physics & Astronomy
University College London
Gower Street
London WC1E 6BT
United Kingdom

Owner

barnabytprowe commented May 10, 2012

(But it won't be for ~1.5 hours as I really do have to go into the office today, and the commute takes a little time.) Once that's done, are you guys happy to merge? I'll let you know.

Owner

rmjarvis commented May 10, 2012

No rush. The important bits are already merged into #103 where I needed it. And I don't think anyone else is waiting on this.

Member

reikonakajima commented May 11, 2012

Everything complies and tests successfully on my Mac OS X 10.7.4 and Ubuntu 10.04.4.

Owner

barnabytprowe commented May 11, 2012

Right, changes pushed and tests run for me, now with Mike's old as well as mine based on the FITS in the repo. I added the IDL script to devutils/external/, which a brief comment about it's use as well as a ref. to this Issue.

Merge at will, as far as I'm concerned!

Owner

rmjarvis commented May 11, 2012

OK. Looks good to me now. Tests passed on Mac and linux for me too.

rmjarvis merged commit 0759776 into master May 11, 2012

rmandelb deleted the #102 branch Jul 4, 2014

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