desihub / redrock

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

NODATA special case leads to truncated fitmethod #301

Closed akremin closed 5 months ago

akremin commented 5 months ago

I don't know the implications of this downstream, but I noticed that the fix in PR #294 seems to have introduced the following warning when redrock is run:

/global/common/software/desi/perlmutter/desiconda/20230111-2.1.0/code/redrock/main/py/redrock/external/desi.py:1037: StringTruncateWarning: truncated right side string(s) longer than 3 character(s) during assignment
  zfit['fitmethod'][ii] = 'NONE'
geordie666 commented 5 months ago

I think the issue was introduced as part of #283 rather than #294. See, e.g., https://github.com/desihub/redrock/blame/main/py/redrock/external/desi.py#L1037. So, I think consulting #283 is the way to go when debugging the warning.

sbailey commented 5 months ago

Context: This was added in #283 (output model fits) to address a conceptual "merge conflict" with #294 (how to flag entirely masked data). The problem was that archtype fits of masked spectra start with fitmethod='ARCH' and then were getting reset to spectype='GALAXY' subtype='', resulting in the model rendering code looking for a galaxy archetype with a blank subtype which doesn't exist. Rather than setting the fitmethod back to PCA or NMF, I invented the concept of fitmethod=NONE which I think is more appropriate for flagging masked data. I think that worked with archetype testing because upstream code had fitmethod='ARCH' resulting 4-character strings prior to NONE being set, but with non-archetype running all upstream options are 3-character PCA or NMF resulting in a truncation here.

Commands for testing, using an interactive GPU node:

salloc -N 1 -C gpu -A desi_g --gpus-per-node=4 -t 04:00:00 -q interactive

srun -n 4 --gpu-bind=map_gpu:3,2,1,0 --cpu-bind=cores rrdesi_mpi --gpu -n 100 \
  -i $CFS/desi/spectro/redux/iron/tiles/cumulative/80605/20210205/coadd-6-80605-thru20210205.fits \
  --model $SCRATCH/redrock/rrmodel-test.fits \
  -o $SCRATCH/redrock/redrock-test.fits
import fitsio
zcat = fitsio.read('redrock-test.fits', 'REDSHIFTS')
set(zcat['FITMETHOD'])
# --> {'NON', 'PCA'}, should {'NONE', 'PCA'}

Thanks @geordie666 for agreeing to fix this even though you weren't the source of the bug.

(FTR: the archetype version of the above rrdesi_mpi call fails for an unrelated reason; I will either fix or post that in a separate ticket)

geordie666 commented 5 months ago

Addressed in #303.

akremin commented 5 months ago

Apologies for blaming without actually checking git-blame! I'm glad it was an easy fix.