-
Notifications
You must be signed in to change notification settings - Fork 24
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
QA frame script #424
QA frame script #424
Conversation
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.
A few minor comments inline. It would also be handy to have an --outdir option for desi_qa_* to be able to write the QA into somewhere other than the production itself. e.g. it's a bit disconcerting that your testing of the dc17a data challenge QA was writing directly into the data challenge itself, for which we probably should have locked down the permissions to prevent accidental overwrite. I'd be inclined to keep the same subdirectory hierarchy under --outdir. Please add that now if you can, or ok to spin to a later update if it is messy for some reason.
# Loop on nights | ||
nights = get_nights() | ||
for night in nights: | ||
for exposure in get_exposures(night): |
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.
it appears that you could just directly jump to
mfile = findfile(root, camera=camera, night=night, expid=fexposure)
if os.path.exists(mfile):
...
i.e. no need to loop over get_exposures(night)
if you already know the exposure number you are looking for. Unfortunately you do still need to loop over nights since we don't have a mapping from exposure -> night.
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.
Well, I still need to catch cases when the exposure folder exists
but not the frame file. Have done so.
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.
Actually, I prefer my way (headed back to it).
This way the error throws that the exposure folder was found
but no frame file in it.
py/desispec/io/frame.py
Outdated
splits = ifile.split('-') | ||
root = splits[0] | ||
camera = splits[1] | ||
fexposure = int(splits[2].split('.')[0]) |
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: os.path.split()
and os.path.splitext()
are more standard ways of doing some of this logic.
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.
ok, implemented os.path.split()
@julienguy -- let me know if you wish to review this before I merge. You may be |
Merging |
Introduces a desi_qa_frame script to generate QA for an input frame file.
Adds new get_nights() and search_for_framefile() methods with tests.
Adds documentation on QA scripts (qa.rst)
Tested successfully on 2% challenge files.