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
Update code to check recent reference runs #42
Conversation
Here are some notes on problems I've found so far that might need to be turned into other tickets:
|
I am now satisfied that the code and data models support (as best as possible) the |
The target and fiberassign files (#44) are now also included. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See minor inline comment about hardcoded "18.2a"
Overall looks good (and extensive!) though I haven't checked in detail. I suggest merging once you have semi-converged on this and then make any further updates in separate smaller PRs rather than trying to get this one to be perfect.
After merging let's make a new desidatamodel tag and comment in the release notes that this was vetted against 18.2a, and then proceed with other updates.
Note that desihub/desispec#545 will move/rename some files that will require further updates to the data model. Let's not wait for that before merging this.
bin/check_minitest.sh
Outdated
check_model DESI_SPECTRO_REDUX ${DESI_SPECTRO_REDUX} > ${SCRATCH}/DESI_SPECTRO_REDUX_${run}.log 2>&1 | ||
check_model DESI_SPECTRO_SIM ${DESI_SPECTRO_SIM} > ${SCRATCH}/DESI_SPECTRO_SIM_${run}.log 2>&1 | ||
check_model -F DESI_TARGET/mtl.rst /global/projecta/projectdirs/desi/datachallenge/reference_runs/18.2a/targets/mtl.fits >> ${SCRATCH}/DESI_TARGET_${run}.log 2>&1 | ||
check_model -F DESI_TARGET/sky.rst /global/projecta/projectdirs/desi/datachallenge/reference_runs/18.2a/targets/sky.fits >> ${SCRATCH}/DESI_TARGET_${run}.log 2>&1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor (for now): should the hardcoded "18.2a" in these lines also be "${run}"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I'll fix that quickly, then merge.
I'm going to merge this one and create new issues and PRs for tracking 18.3-specific changes. |
This PR is related to #40, but does not necessarily close it.
desiutil.log
instead ofwarnings.warn
.I will have additional commits to add to this, but I wanted to get it into the pipeline.