Tickets/dm 10225 #2
Conversation
| @@ -4,3 +4,4 @@ | |||
| config.log | |||
| ups/*.cfgc | |||
| python/lsst/obs/ctio0m9/version.py | |||
| examples/.ipynb_checkpoints/* | |||
There was a problem hiding this comment.
Please add carriage return at the end of the line.
config/ingest.py
Outdated
| 'expTime': 'double', | ||
| 'object': 'text', | ||
| 'imgType': 'text', | ||
| 'wavelength': 'double', | ||
| } |
There was a problem hiding this comment.
In general I find it best to not align dictionary keys. If another key is added in the future that is longer than any of the current keys then you have to realign everything. But I don't think that our style guide or PEP8 forbids this, so feel free to do as you choose...
There was a problem hiding this comment.
We do not align because the diffs look terribly confusing when you add a new key that is longer anywhere to the dict. PEP8 explicitly says not to align variable definitions because of this, and I believe this code will trigger an E241 error. Has this code been checked with flake8?
There was a problem hiding this comment.
Understood. I shall un-align everything to use a single space so that people aren't tempted to keep this going in future.
python/lsst/obs/ctio0m9/ingest.py
Outdated
| """Get the image type (e.g. bias, dark, flat etc) from the header | ||
| Translator function derived from a very small dataset from the observatory | ||
| i.e. may well need adding to when new string values are found | ||
| """ |
There was a problem hiding this comment.
I think the docstring should contain a first line that gives a general description of the method, a blank line, and then an extended definition. You should also be specific about what type of object md is.
There was a problem hiding this comment.
Ideally docstrings are recommended to use the Numpydoc format, since there are plans to use sphinx and keep the code uniformly documented, but as I'm sure you've noticed this is not required and not always done.
There was a problem hiding this comment.
Looks good, although from this docstring I'm still not sure what type of object it expects for metadata.
python/lsst/obs/ctio0m9/ingest.py
Outdated
| if val in conversion: | ||
| return conversion[val] | ||
| else: | ||
| return 'NOT_TRANSLATED' |
There was a problem hiding this comment.
Should you raise a warning or exception, or is it ok to just return a 'NOT_TRANSLATED' string?
There was a problem hiding this comment.
I agree that an exception or None would be better than a magic string.
There was a problem hiding this comment.
OK, I've added a warning, for now. It is known that this translator will need to be added to though, as/when new values are found in the metadata.
There was a problem hiding this comment.
I agree with @RobertLuptonTheGood , this should probably also return None.
There was a problem hiding this comment.
OK, changed to None
python/lsst/obs/ctio0m9/ingest.py
Outdated
| """Get the monochromator wavelength from the header, for flats only. | ||
| Method will need ammending if/when this value is written elsewhere | ||
| """ | ||
| val = md.get("OBJECT").rstrip().lstrip() |
There was a problem hiding this comment.
Please update docstring as specified in previous comment.
python/lsst/obs/ctio0m9/ingest.py
Outdated
| return float('nan') # defaults to NaN if not a flat | ||
| if val[0:3].isdigit(): | ||
| wavelength = float(val[0:3]) | ||
| if wavelength<300: #blindly using the first 3 digits, so if >=1000nm could be bad |
There was a problem hiding this comment.
It's probably better to be more precise if the number of digits is not fixed.
It looks like:
import itertools
try:
wavelength = int("".join(itertools.takewhile(str.isdigit,val)))
except ValueError:
return np.nan
or
import re
try:
wavelength = int(re.match('\d+', val).group())
except ValueError:
return np.nan
will work.
There was a problem hiding this comment.
If the number of digits is not 3 or 4 then something is very wrong. I've changed the logic to deal with these cases, and in doing so, have avoided using ugly catch-all regex stuff.
python/lsst/obs/ctio0m9/ingest.py
Outdated
| wavelength = float(val[0:3]) | ||
| if wavelength<300: #blindly using the first 3 digits, so if >=1000nm could be bad | ||
| if wavelength<300 or wavelength>1150: #We don't know what might be stored here, | ||
| #so a little sanity checking is good |
There was a problem hiding this comment.
Looks good, this is certainly more readable than the code I suggested. It might be good to define MIN_WAVELENGTH=300 and MAX_WAVELENGTH=1150 at the beginning of the code in case these numbers ever need to change and/or are used anywhere else in the code.
There was a problem hiding this comment.
I don't think those need defining - they're properties of the band-gap of silicon, and so not only will not change, but this is for a specific obs_package, and so won't ever be upgraded to use detectors outside the range.
mfisherlevine
force-pushed
the
tickets/DM-10225
branch
from
7e23a04
to
ed42d0e
Compare
No description provided.