desihub / desisurvey

Code for desi survey planning and implementation
BSD 3-Clause "New" or "Revised" License
2 stars 7 forks source link

use desimodel.footprint.pass2program if available #68

Closed sbailey closed 7 years ago

sbailey commented 7 years ago

This PR is a followup to #67, which hardcoded the tile pass -> program mapping since desimodel didn't provide that directly. In the meantime, desihub/desimodel#67 adds desimodel.footprint.pass2program to do this based upon the desi-tiles.fits tile definition file.

This PR uses desimodel.footprint.pass2program if available (i.e. desimodel >= 0.9.1 is installed), and defaults to the previous hardcoded mapping since that is still valid for desimodel < 0.9.1. If we never change the mapping, then this won't matter. But if we do, now we just have to change the desimodel desi-tiles.fits file and the correct new mapping with automatically be used.

dkirkby commented 7 years ago

There are other places where pass2program should be used in desisurvey. Do you want to merge this asap, or can it wait until I track those down?

weaverba137 commented 7 years ago

Please note that I inadvertently pushed a fix to the license file to this branch, instead of master.

sbailey commented 7 years ago

I'd like to suggest one more change for Progress.get_exposures() to be included in this PR, but it will have some minor downstream effects to checking first before implementing.

Currently exposures['NIGHT'] and exposures['PROGRAM'] are created with dtype 'S8' and 'S6' meaning that in python 3 elements should be compared to bytes not str. However, if you write the exposures table to disk and read it back in, astropy.table.Table "helpfully" upconverts the bytes on disk into unicode strings, reversing how the comparisons should be done. i.e. it doesn't survive a roundtrip to FITS when using astropy.table.Table:

exposures = p.get_exposures()
isdark = exposures['PROGRAM'] == b'DARK'  #- yes
isdark = exposures['PROGRAM'] == 'DARK' #- no
exposures.write('exposures.fits')
exp2 = Table.read('exposures.fits')
isdark = exp2['PROGRAM'] == b'DARK'  #- no
isdark = exp2['PROGRAM'] == 'DARK' #- yes

exp2['PROGRAM'] == exposures['PROGRAM']  #- False  (not even [False, False, ...])
exp2['NIGHT'] == exposures['NIGHT']  #- False  (not even [False, False, ...])

As a result, client code has to know if it was receiving the Table as read from disk or whether it was receiving the Table as created by Progress.get_exposures. Ugh.

Suggestion: directly create these columns as dtype U8 and U6 so that the returned Table has the same dtype as what you would get if you write it to fits and read it back in, and all client code can just treat these columns as strings.

[I acknowledge D. Kirkby's earlier comment in another thread that working with strings in tables is a pain. Agreed, especially with python 3.]

weaverba137 commented 7 years ago

@sbailey, I ran this branch through the minitest notebook. It's working fine. But we still need to answer @dkirkby's question about other places that pass2program should be used.

As far as setting the dtype goes, that's fine with me, though are we enforcing that standard throughout the DESI package system? Is there an existing unit test for the round-trip?

sbailey commented 7 years ago

Good question about what we do elsewhere for bytes vs. str vs. unicode. desispec.io.empty_fibermap() may be the closest analogy for elsewhere in the code where we want to create an object on-the-fly and have it natively work for str comparisons and survive a round-trip to disk. In that case, we created string columns with dtypes like (str, 8) which works for both python 2.7 and 3.5. My original proposal here of using U8 would not work for python 2.7 (deprecated, but not yet dead...)

I don't think we have a unit test for this roundtrip (otherwise it would have failed). I'll add that.

Let's not wait for fixing up all possible places that could/should use pass2program. That will only matter if we ever change the mapping of pass numbers to program names in desimodel.

sbailey commented 7 years ago

Please re-review. I've made the dtype change, added an exposures roundtrip to FITS test, and the tests pass for both python 2.7 and 3.5. I'm happy to proceed with merging this and update other pass2program cases later.