-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NODATA special case leads to truncated fitmethod #301
Comments
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. |
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:
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) |
Addressed in #303. |
Apologies for blaming without actually checking git-blame! I'm glad it was an easy fix. |
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:
The text was updated successfully, but these errors were encountered: