When loading tile file, check existance before looking in data folder

[tskisner]

Currently loading a tile file in the current working directory fails, because it treats all bare filenames as relative to the package data directory:

>>> import desimodel.io
>>> tiles_data = desimodel.io.load_tiles(tilesfile='tiles_skysub.fits', cache=False)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/kisner/software/desi/desimodel/master/lib/python3.6/site-packages/desimodel-0.9.9.dev464-py3.6.egg/desimodel/io.py", line 180, in load_tiles
    with fits.open(tilesfile, memmap=False) as hdulist:
  File "/home/kisner/software/desibase/lib/python3.6/site-packages/astropy/io/fits/hdu/hdulist.py", line 151, in fitsopen
    lazy_load_hdus, **kwargs)
  File "/home/kisner/software/desibase/lib/python3.6/site-packages/astropy/io/fits/hdu/hdulist.py", line 390, in fromfile
    lazy_load_hdus=lazy_load_hdus, **kwargs)
  File "/home/kisner/software/desibase/lib/python3.6/site-packages/astropy/io/fits/hdu/hdulist.py", line 1039, in _readfrom
    fileobj = _File(fileobj, mode=mode, memmap=memmap, cache=cache)
  File "/home/kisner/software/desibase/lib/python3.6/site-packages/astropy/utils/decorators.py", line 503, in wrapper
    return function(*args, **kwargs)
  File "/home/kisner/software/desibase/lib/python3.6/site-packages/astropy/io/fits/file.py", line 178, in __init__
    self._open_filename(fileobj, mode, overwrite)
  File "/home/kisner/software/desibase/lib/python3.6/site-packages/astropy/io/fits/file.py", line 555, in _open_filename
    self._file = fileobj_open(self.name, IO_FITS_MODES[mode])
  File "/home/kisner/software/desibase/lib/python3.6/site-packages/astropy/io/fits/util.py", line 388, in fileobj_open
    return open(filename, mode, buffering=0)
FileNotFoundError: [Errno 2] No such file or directory: '/home/kisner/software/desi/desimodel/master/data/footprint/tiles_skysub.fits'

Instead it should check if the path exists first. I have already fixed this in a branch. PR coming.

[sbailey]

The intended usage if you want to force it to get a local copy is:

tiles_data = desimodel.io.load_tiles(tilesfile='./tiles_skysub.fits', cache=False)

In particular we don't want cases like

tiles_data = desimodel.io.load_tiles(tilesfile='desi-tiles.fits', cache=False)

to be ambiguous about whether a local copy is overriding the standard copy or not.

i.e.

• no path = get it from $DESIMODEL/data/footprint
• with path = get it from the location specified by the path

Would that meet your needs?

[tskisner]

I guess that behavior was quite surprising to me. The docstring says:

tilesfile : (str)
        Name of tiles file to load; or None for default.
        Without path, look in $DESIMODEL/data/footprint, otherwise load file.

I passed in the name of a file to load and it raised an exception because it was looking in the package data directory. If this behavior is intended, then we should at least change the docstring, and I will do additional checks in fiberassign to warn the user what is happening if they pass the name of a tile file that exists in the current working directory.

[sbailey]

I think the docstring reflects the intended usage (which I agree is not what was actually implemented). A key detail is that without a path, $DESIMODEL/data/footprint/filename.fits still trumps local ./filename.fits. I think PR #98 does the opposite of that (local copy trumps $DESIMODEL copy).

I'm ok with falling back to checking for a local copy if a $DESIMODEL copy doesn't exist, but I don't think we want the reverse.

[moustakas]

I'm ok with falling back to checking for a local copy if a $DESIMODEL copy doesn't exist, but I don't think we want the reverse.

I think I disagree with this statement. Presumably, all tile files kept in $DESIMODEL will have fixed names and a well-established provenance (and any missing files are presumably due to a borked installation of desi software, but if that happens then that's probably the least of the user's worries), and all file names can be generated via a small number of keywords (e.g., bright=True).

Alternatively, anyone explicitly using the tilesfile argument presumably wants to use that specific file, whether it's a local copy (without the absolute path) or a non-standard file in a non-standard location. (I explicitly ran into this issue working on survey simulations for the SV data challenge, using a non-standard and not-final tile file, and had to do some trickery to get everything working with "my" file.)

On the other hand, if $DESIMODEL will have N different official tile files (where N>>1) then perhaps my proposal is not a good one.

My $0.02.

[sbailey]

At minimum we should expect $DESIMODEL to have tiles files for commissioning, sv, and the main survey. Beyond that, I'm not sure, but I would like to be able to retain the ability to specify a specific filename in $DESIMODEL without making special case options for each one, and without risking having a local file by the same name silently (and perhaps accidentally) override it.

@moustakas did you have to do "trickery" beyond just "./my-tiles-files.fits" instead of "my-tiles-file.fits"? Having to add "./" doesn't seem so bad to me. Updating the code to match what the docstring says it does also seems good. But if some chain of code is stripping off the "./" and then borking io.load_tiles and causing you to resort to things like temporarily resetting $DESIMODEL, then that would indeed be bad.

[tskisner]

There needs to be some disambiguation between a file in the current working directory and a file with the same name in desimodel footprint.

What about raising an exception in the case tilesfile="blah.fits" and blah.fits exists both locally and in desimodel. This would force the user to either move their local file out of the way (if they did not want to use it) or be more explicit by specifying "./blah.fits".

[tskisner]

I just pushed a change to the PR:

• Check if the local file exists
• Check if the file in $DESIMODEL/data exits
• If neither exists, raise an exception
• If one exists, read it
• If both exist, print a warning and choose the one from $DESIMODEL

Does that sound reasonable?

[sbailey]

@tskisner sounds good. Thanks.

[sbailey]

implemented in #98. Closing.