Raise error if fits.open(memmap=True) is impossible. - #2298

This is a possible solution to #2295, if there is interest

The tests fail because the new ValueError is triggered on test images. I'm not sure what the "correct" behavior should be -- I'm happy to revise the PR to make the behavior conform to what a dev thinks should happen.

What if some HDUs are memmappable and others aren't? It's conceivable that a user would want to open a FITS file to read a memmapped HDU and not care that others aren't memmappable. Is it possible to move this exception to the point at which the data for an HDU is actually read in from the file?

The exception is triggered at the moment when an HDU data property is accessed:

d = fits.open(pth)  # no error
hdu = pth[0] # no error
hdu.data  # error

I haven't looked at the test failures carefully enough to see if there are situations where data is accessed implicitly at open time.

Oh -- my bad. Ignore my comment -- it seems to work as I would expect.

Yeah, still not sure how I feel about this. The thing is, using mmap for pyfits didn't used to be a thing at all--either the file fit in memory or it didn't (CFITSIO has its own tricks to deal with this). But so in general users just don't care--it's something that I made to work as a way to silently improve matters in the average case, though it currently isn't guaranteed to work in all cases. I think a better solution here might just be better documentation as two when it will benefit and when it won't.

memmap is invaluable for taking views into large fits files, and it's hugely problematic that the presence of BLANK currently makes it impossible to use memory mapping. There are a dozen of us at an ALMA software workshop, and we've been forced use this PR as a workaround to work with 30GB ALMA cubes (which set BLANK in error to boot-- it's not actually used to mask any of the floating point values)

From my perspective, it seems really important to permit memory mapping in the presence of BLANK -- I don't care if this is as a separate kwarg, or bundled into do_not_scale_image_data.

I also consider it a bug that I can explicitly set memmap to True and silently not get a memory mapped file (and even worse, run out of RAM).

What are your feelings about a 3-tier system for the memmap kwarg -- a default None that silently uses memmap if possible and an array if not, True which uses a memmap or raises an exception, and False which always uses an array? This would give people some guarantees about RAM consumption, without raising errors or warnings for the average user.

FWIW, I'm +1 on the idea of making sure that if memmap is explicilty set to True, it should either work or raise an exception. I'd also vote that BLANK should be ignored for floating-point data.

I'll need to give it a little more thought, but I think I'm okay with that. I'd also definitely accept a patch that checks the user's available system memory and compares it against the size of the data so as to not even try to load the data into memory if it looks like it won't be able.

Note also that for images anyway the .sections feature exists. This allows pulling only limited slices of the image into memory at once (regardless of whether mmap is being used). An equivalent for tables doesn't exist yet but I plan for it to (this would enable things like row-based iteration that is so sorely missing).

It might be better eventually to drop the .sections feature altogether and use a similar feature transparently even when mmap is not in use, but doing that right also involves on some level a custom paging system that works transparently with Numpy.

@embray I've removed the BLANK stuff here, and switched to the three tier memmmap mode discussed above. I'll tackle blank elsewhere

@ChrisBeaumont - is this still relevant, or was it fixed by another PR? If this is still needed, can you rebase?

This is still relevant; we just haven't agreed on a resolution.

Rebased (though like @embray mentions, this isn't necessarily the final solution. I'm still in support of it)

Just to briefly clarify my position on this, as the discussion has been spread around a few places: I completely agree with this approach in principle. But in practice I know from experience that it will cause trouble. There used to be a warning when mmap was enabled but would not be useful for that particular data, and it became a pain for users of software that uses pyfits, because those warnings would be sent to users who don't actually care about such details. This meant it was up to application developers to check/anticipate when it would or would not be safe to use mmap with pyfits, which itself is an undue burden.

In most practical applications it has been less of a burden to explain why in some cases using a particular FITS file requires more memory than others. So in general I'd rather just avoid the problem while I work toward an overall better solution for transparently mapping raw FITS data to the data seen by users.

Thanks -- to summarize my perspective:

The solution in this PR doesn't add any burden by default (no warnings or error messages should be displayed if the user omits the memmap kwarg -- memmap is used if possible, and transparently ignored if not). If a user or application developer explicitly provides memmap=True, however, this is interpreted as a requirement -- the result will be a memory mapped array or an exception

I think this is the right burden balance to strike. The application developer shouldn't have to worry about memory issues in the default case, but he/she should be able to worry about it if the application is designed for large datasets. I think most application developers would agree that raising an exception is better than running out of memory.

I'm in favor of @ChrisBeaumont's approach here - only advanced users will have existing code with memmap=True, so I think the potential for confusion is low.

Ah right, got it. In that case I think I'm okay with that--but let me test out the change in PyFITS first.

Sorry I sort of dropped the ball on this one. I'm going to see if I can get it integrated locally to see what effect it has on some of our software.

@embray - any updates on this? I'd like to also have this behavior available for software I'm working on.

I'm okay with merging this (with the proper changelog entry, etc.) provisionally and just seeing how it works out in practice for a bit. I think it should be fine though.

@ChrisBeaumont - could you add a changelog entry for 1.0?

Ok, changelog entry added

@ChrisBeaumont - thanks!

@embray - what else is needed to merge this?