Added PDS Image save support.

bvnayak commented 8 years ago

refs#45

bvnayak commented 8 years ago

This pull-request contains the basic 'Save' support. We are about to add gz and bzip2 write support.

godber commented 8 years ago

If overwrite=False and a file with the name file.IMG exists, and you try to save a file with the name file.IMG what will this code do? Will it allow the user to accidentally overwrite the file? It looks like it to me. Thats not good. Please fix.

godber commented 8 years ago

These were my comments on the previous PR, please make sure they are addressed here:

@bvnayak you can ignore these comments until next week, since I know you're studying.

I have a number of concerns with this PR:

save() is implemented in Image but currently only handles saving PDS3 images. What happens when a user tries to save an Isis cubefile? Probably not the right thing. So we can either leave save() up here and raise an exception when an Isis cubefile is being saved or we can move save() down to the PDS3Image class. This implementation looks pretty PDS3 specific so maybe it should be moved down into pds3image.py.
The overwrite keyword doesn't do what I had intended for it to do. save() should test to see if a file matching file_to_write is present, if it is present and overwrite=False then an exception should be raise with the message "File already exists: filename". ONLY if overwrite=True should the file be overwritten. The whole point is that save() should never overwrite an existing file without explicit permission. This can probably be accomplished in the missing else: branch of the if overwrite: test.

Lastly, I've realized that many of our tests for pds3image are implemented incorrectly. They should be testing image.label and image.data. They mostly test the convenience properties that should maybe be considered internal and changed to band -> _band. But at the very least, label values need to be tested explicitly since that's the interface users should be using. Please modify your new tests here to include tests of image.label and image.data. I will file another ticket to change the older tests.

bvnayak commented 8 years ago

I have modified the properties in the "write_test". I will resolve the issues in the comment above and tests properties in this pull request.

bvnayak commented 8 years ago

/home/bvnayak/temp/sample/sample_3_band.IMG File was saved using planetaryimage and is supported by MERView.

godber commented 8 years ago

I am going to accept this MR after I review its functionality once more. There are several reasons this has sat around for so long and I will share some of those in an issue that I post sometime in the future. The two MAIN reasons are

Seeing this implemented I realized how we have a situation where the image data and metadata can be out of sync, for instance someone could change the type of the data from FLOAT to INT and the metadata won't correctly reflect that change without also correcting the metadata. Yes, this only occured to me now (where now was like October).
@wtolson has an interesting branch which changes everything https://github.com/wtolson/planetaryimage/tree/plugin-refactor. We both have misgivings about whether or not we should even be unifying PDS and ISIS Cubefile IO.

godber commented 8 years ago

I've tested all of the files in tests/mission_data and every one that can be opened can be saved and the new file opened.

tests/mission_data/1p134482118erp0902p2600r8m1-new.img
tests/mission_data/1p190678905erp64kcp2600l8c1-new.img
tests/mission_data/2m132591087cfd1800p2977m2f1-new.img
tests/mission_data/2p129641989eth0361p2600r8m1-new.img
tests/mission_data/h58n3118-new.img

planetarypy / planetaryimage

Added PDS Image save support. #52