desihub / redrock

Redshift fitting for spectroperfectionism
BSD 3-Clause "New" or "Revised" License
22 stars 13 forks source link

inconsistent fibermap written to zbest file #184

Closed moustakas closed 5 months ago

moustakas commented 3 years ago

@sbailey

This is probably a feature rather than a bug but I wanted to report it because it tripped me up. I'm not sure if we should do anything about it.

The fibermap written to the zbest file is different than what's in the coadd files, even when fitting to the coadds in redrock (which is the default, i.e., unless --allspec is used, which we don't in production). E.g.,

import fitsio
import numpy as np
from astropy.table import Table
zbfile = '/global/cfs/cdirs/desi/spectro/redux/daily/tiles/80605/20201215/zbest-9-80605-20201215.fits'
zb = Table(fitsio.read(zbfile, ext='ZBEST'))
fmap = Table(fitsio.read(zbfile, ext='FIBERMAP'))
cofmap = Table(fitsio.read(zbfile.replace('zbest-', 'coadd-'), ext='FIBERMAP'))
print(len(zb), len(cofmap), len(fmap))
print(np.array(cofmap.colnames)[np.logical_not(np.isin(cofmap.colnames, fmap.colnames))])

500 500 2500
array(['MEAN_DELTA_X', 'MEAN_DELTA_Y', 'COADD_NUMEXP', 'RMS_DELTA_X',
       'RMS_DELTA_Y', 'FIRST_NIGHT', 'LAST_NIGHT', 'NUM_NIGHT',
       'FIRST_EXPID', 'LAST_EXPID', 'NUM_EXPID', 'FIRST_TILEID',
       'LAST_TILEID', 'NUM_TILEID', 'FIRST_FIBER', 'LAST_FIBER',
       'NUM_FIBER'], dtype='<U21')

Semi-related I also noticed that we pass the spectra- files to desi_nightly_redshifts in production, when I think we should be passing the coadd- files-- https://github.com/desihub/desispec/blob/master/bin/desi_nightly_redshifts#L145-L155

If we passed the coadd- files then I think this issue would be moot (since I think that redrock simply passes forward the fibermap table it inherits from the data on-disk).

sbailey commented 3 years ago

Related to #189 (performance on cosmics-masked coadds) and desihub/desispec#1104 (fibermap format in general for coadds vs. not). I'm punting this issue to the Denali release since we're not converging with a clean answer for Cascades, so leaving it as-is (albeit admittedly messy).

moustakas commented 5 months ago

I verified that this is no longer an issue.