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
DM-5466: obs_decam calibration ingest uses fragile relative paths #104
Conversation
Please use a clearer summary line in your git commit. Mentioning "code changes" isn't helpful because pipe_tasks is almost all code. And mentioning obs_decam doesn't help either because there's a bunch of stuff there. Be specific, e.g., "ingestCalibs: allow copy/link of calib into repo". |
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.
Please do not merge this until you have confirmed that it will work for the usual case (ingesting calibs that have been created within the calib data repo) without any operational changes.
ccdnum = info["ccdnum"] if info.get("ccdnum") else False | ||
if (ccdnum == False) and (calibType == "flat" or calibType == "bias"): | ||
info["ccdnum"] = 1 | ||
calibType = "cp" + calibType[0].upper() + calibType[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.
"cp"
and "ccdnum"
are DECam-specific, and don't belong in pipe_tasks.
@return Destination filename | ||
""" | ||
#raw = butler.get("raw_filename", info)[0] # this definitely won't work | ||
#info["ccdnum"] = 1 # this is a decam-only hack for CP products |
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.
Remove commented-out lines.
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.
This pull request is very much a placeholder at present; I will definitely test as you suggested on calibs that have been created with pipe_base and clean up commented-out lines etc.
if (ccdnum == False) and (calibType == "flat" or calibType == "bias"): | ||
info["ccdnum"] = 1 | ||
calibType = "cp" + calibType[0].upper() + calibType[1:] | ||
#import pdb; pdb.set_trace() |
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.
Remove.
outfile = self.parse.getDestination(args.butler, fileInfo, infile) | ||
ingested = self.ingest(infile, outfile, mode=args.mode, dryrun=args.dryrun) | ||
if not ingested: | ||
continue |
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.
Warning?
@@ -198,6 +225,10 @@ def run(self, args): | |||
self.log.warn(str("Skipped adding %s of observation type '%s' to registry" % | |||
(infile, calibType))) | |||
continue | |||
outfile = self.parse.getDestination(args.butler, fileInfo, infile) |
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.
In recent work, I found it helpful to have the filename logged here, so it's clear that it's doing something, and what it's operating on. Would you add that, please?
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.
This information is printed out as part of the IngestTask, but I can change it to a log info entry.
80809a2
to
3a1dd01
Compare
Donut fitting is now done iteratively, with the maximum Zernike coefficient in a fit allowed to increase in each iteration (controlled by a config list). The fit results from the previous iteration is used as the starting value of the next iteration.
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.
Some commits should be squashed. Otherwise looks fine to me.
Somehow @jmeyers314's commits got merged to master as well in this PR? |
I assume that must be my fault. I'm not 100% certain, but my best guess is this was when I was trying to rebase my user branch on top of the latest weekly. I must have rebased off of this commit instead somehow ?? Anyway, sorry about that. How can we undo this? |
I've reverted the commits on master. Well spotted, @hsinfang! |
It may have been my fault @jmeyers314, but I'm not sure either :) I intended to rebase off master but may have used one of your commits by mistake. At any rate, thanks for the quick fix! |
No description provided.