Repository navigation
Use rustfits or astropy remote file access when possible. - #178
Conversation
erykoff
changed the title
|
|
||
| self.fits_object = rustfits.FITS(filename, mode=rustfits_mode) | ||
| if mode == "r" and protocol != "file": | ||
| self.fits_object = rustfits.FITS(filename, mode=rustfits_mode, remote="ranged") |
There was a problem hiding this comment.
Is this defaulting to always using ranged? I think often this is not the best choice, much faster to use the default "download all and work with it in memory"
There was a problem hiding this comment.
Hmm. Part of the problem is that only rustfits supports this. The other thing is that I was thinking that if somebody is handing a remote URI then they are asking to read it remotely (for good or ill). I also worry about trying to get too many parameters in at the top and passing them all down ... but if you think it's necessary I could.
There was a problem hiding this comment.
what is only supported by rustfits?
There was a problem hiding this comment.
Right now I support various fits backends, but I am thinking that I should remove that ... or at least remove fitsio because I do require rustfits. So I'll do that cleanup on this PR.
But I also support astropy fits reading which is currently required for remote access via S3 or google cloud or webdav (I don't think rustfits supports these or it would be a great surprise). And only rustfits will "download the file to a temp cache and then read it". I personally find that behavior surprising. I would instead expect that if I give a remote protocol I want to read it remotely.
There was a problem hiding this comment.
Getting this to work took some real code and decisions, where as the download version didn't take any.
Also the download is what cfitsio does.
From that point of view, ranged access not being default isn't a surprise, the ranged read existing at all is the nice addition (thanks for prompting it). It is nice astropy does it too.
Why does S3, google cloud, webdave require anything other than authentication? rustfits does support that through the keyword
There was a problem hiding this comment.
| fitsio>=1.4.0 | ||
| rustfits = | ||
| rustfits>=0.1.5 | ||
| rustfits>=0.1.7 |
There was a problem hiding this comment.
I think ranged landed in 0.1.8
fjaviersanchez
left a comment
fjaviersanchez
left a comment
There was a problem hiding this comment.
The changes and tests look good to me. I just left a comment regarding astropy.io.fits but it shouldn't prevent you from merging this one. Thank you!
| firstrow = (hdu.get_dims()[0], ) | ||
| hdu.write(data, start=firstrow) | ||
| elif has_astropy: | ||
| elif self._use_astropy: |
There was a problem hiding this comment.
I might be missing something, but I think astropy.io.fits does allow appending. I think the only limitation is if the data block is compressed in certain formats. I think that you can either open the fits on append mode and append to the HDUList object or use fits.append.
There was a problem hiding this comment.
As far as I know, astropy fits does not allow you to stream append with tile compression, which is what I'm looking for here. You can only append a whole new HDU.
c9a6a50 to
84b28ad
Compare
No description provided.