esheldon / fitsio

A python package for FITS input/output wrapping cfitsio
GNU General Public License v2.0
134 stars 58 forks source link

Suggestion: Emit warning when fitsio.write doesn't overwrite a file since clobber=False. #336

Open rmjarvis opened 3 years ago

rmjarvis commented 3 years ago

I've been bitten by this fitsio feature multiple times: When calling fitsio.write(file_name, data), if the file exists already, fitsio will append a new extension rather than overwrite the file.

I get that this is intended behavior, and is correctly documented. There is also a simple parameter to change it clobber=True.

However, I think the UI could be improved by adding a warning when this occurs, since if you're expecting the file to be overwritten, it seems like it did nothing, since the first, default extension is unchanged. So reading in that file somewhere else ends up with the same, old information, not the new data you meant to write to the file.

Therefore, I'm suggesting that, when clobber=False and the file exists, fitsio emit a warning letting the user know what fitsio is doing, since I'm guessing I'm not the only user who finds this a bit counter-intuitive.

esheldon commented 2 years ago

This is default cfitsio behavior, and is also the behavior of mwrfits from IDL. I think it is terrible but I was trying to stick with what people would expect.

Now that the user base is mostly people who never used either cfitsio or mwrfits, it is surprising! There is no win in these situations.

The problem with the warning is when someone does want to add a new extension (not uncommon!) they will get crap in their logs.

rmjarvis commented 2 years ago

I think usually when you would be intentionally adding an extension, you'd probably name it. (Not required of course, but it is good practice.) So maybe only emit a warning if clobber=False AND extname is None?

Or possible even better, have the default clobber be None, rather than False, and then only emit a warning if clobber is None and extname is None with part of the message saying that explicitly using clobber=False will silence the warning.

rmjarvis commented 2 years ago

(Good API design is hard...)

ntessore commented 2 years ago

I just had a big argument with this one. I would like to propose raising an exception by default if a file exists for writing. This would probably be the path of least unexpected behaviour for most people.

In combination with that, the default could be clobber=None which raises the exception. The user can then specifically set clobber=False to force appending, or clobber=True to force clobbering.

Even better would be to introduce new explicit file modes r, w, a in both the FITS opener and the convenience routines (and keep rw for the current behaviour). Then we could say very specifically what we want the code to do (explicit is better than implicit), e.g. fitsio.write(file, data, mode='a').