DM-18739: allow ingesting defects with a slim wrapper on ingestCalibs #295
Conversation
SimonKrughoff
force-pushed
the
tickets/DM-18739
branch
from
31367f3
to
46ba9c9
Compare
|
|
||
| self.conn = sqlite3.connect(self.updateName) | ||
| if not haveTable or forceCreateTables: | ||
| createTableFunc(self.conn) | ||
| createTableFunc(self.conn, forceCreateTables=forceCreateTables) |
There was a problem hiding this comment.
Please consider updating the doc strings in this file to sphinx.
| if table is None: | ||
| table = self.config.table | ||
| cmd = "SELECT name FROM sqlite_master WHERE type='table' AND name='%s'" % table |
There was a problem hiding this comment.
I strongly suggest you leave %s in the string and pass the argument separately to cursor.execute -- both here and in the other simple sql commands below (it may be too much work for the more complicated command). This allows execute to sanitize the inputs for you, to prevent SQL injection. For example:
cmd = "SELECT name FROM sqlite_master WHERE type='table' AND name='%s'"
cursor.execute(cmd, table)
In cases where you don't think it's worth the effort please consider using f strings, as they are easier to read.
| @@ -1,6 +1,7 @@ | |||
| import collections | |||
| import datetime | |||
| import sqlite3 | |||
| from dateutil import parser | |||
There was a problem hiding this comment.
Consider import dateutil... dateutil.parser.parse(...)
| @@ -38,6 +39,8 @@ def getCalibType(self, filename): | |||
| obstype = "fringe" | |||
| elif "sky" in obstype: | |||
| obstype = "sky" | |||
| elif "defects" in obstype: | |||
| obstype = "defects" | |||
There was a problem hiding this comment.
This is getting a bit long and the stutter worries me (a typo could easily creep in). Consider a loop, such as:
remap_dict = {"zero": "bias"}
for name in ("flat", "zero", "bias", "dark", "fringe", "sky", "defects"):
if name in obstype:
return remap_dict.get(name, name)
|
|
||
|
|
||
| class IngestDefectsTask(IngestCalibsTask): | ||
| """Task that generates registry for calibration images""" |
There was a problem hiding this comment.
This doc string is wrong because this only handles defects. Consider copying the "run" doc string, since "Task that..." is not very helpful (it is clearly a task) and doc strings are supposed to be in active voice.
At one time task doc strings were supposed to be extensive, with sections for examples, debugging (which debug flags are supported)...is that still the case?
| args.validity = None # Validity range is determined from the files | ||
| IngestCalibsTask.run(self, args) | ||
| except Exception: | ||
| raise(Exception) |
There was a problem hiding this comment.
This except/raise does nothing useful. Please delete it.
| ("varStdDev", varStdDev, 55.98012493452948), | ||
| ("psfIxx", psfIxx, 2.769679536557131), | ||
| ("psfIyy", psfIyy, 2.2013649766299324), | ||
| ("psfIxy", psfIxy, 0.14797939531970852) |
There was a problem hiding this comment.
You are only testing to 7 places; please consider shortening these values so they only have a few extra digits. It would make the test a bit easier to read (the interesting digits are lost in the middle). I realizing rounding by hand is no fun, but you could easily print the data to the desired precision and copy/paste or use Python to generate this whole block.
| choices=[None, "bias", "dark", "flat", "fringe", "sky", "defect"], | ||
| help="Type of the calibration data to be ingested;" | ||
| " if omitted, the type is determined from" | ||
| " the file header information") |
There was a problem hiding this comment.
If calibType can no longer be specified on the command line, I am pretty sure decam fringe ingestion will break, although it is already pretty broken (it must presently be run with mode=skip and the files must be moved/copied/linked manually to the calib repo).
SimonKrughoff
force-pushed
the
tickets/DM-18739
branch
2 times, most recently
from
43305c4
to
18bf1a9
Compare
Because the data ids are parsed out of the header for calibrations produced by the calibration products pipeline, this will not work for user curated data. This removes extra code used only for defects.
It turns out that getDestination doesn't respect the calibType argument anyway, so this option has been removed.
Note I had to call them "deflects" so the mapper special casing didn't shadow my attempts. Also, this uses a put to persist the defects to avoid an intermediate file. This is not the standard practice as usually calibs are on disk and are simply linked into a data repository.
os.rename only works within a single physical drive. Ths shutil.move function will copy and then delete the remaining file.
The binary data in obs_base were updated several years ago, but the defects were not updated to agree. Now that those defects have been updated, the nominal values in the test must change as well.
I believe this and the next commit fix DM-16817
SimonKrughoff
force-pushed
the
tickets/DM-18739
branch
from
4795236
to
9a280b8
Compare
I believe this also addresses DM-16817, so am asking for review of that with this same ticket.