DM-18739: add code to standardize defects #143
Conversation
SimonKrughoff
force-pushed
the
tickets/DM-18739
branch
from
e69e5ae
to
29a524c
Compare
SimonKrughoff
force-pushed
the
tickets/DM-18739
branch
2 times, most recently
from
bc39baf
to
0a55c53
Compare
python/lsst/obs/base/read_defects.py
Outdated
| finst = md.get('INSTRUME') | ||
| fchip_id = md.get('DETECTOR') | ||
| fcalib_date = md.get('CALIBDATE') | ||
| if not (finst, int(fchip_id), fcalib_date) == (instrument, chip_id, valid_start.isoformat()): |
There was a problem hiding this comment.
Note that the failure message will be difficult to parse if any of the metadata is missing. Consider if None in (finst, fchip_id, fcalib_data): raise... so the error message says which file is the problem.
There was a problem hiding this comment.
I want to check for missing metadata as well as differences inferred from the path vs that encoded in the file metadata. I added a line to print the file it is reading from. I hope that's ok.
There was a problem hiding this comment.
I still recommend you include the file name when you raise an exception. I think that will be clearer than printing each file as you go and hoping the user realizes it's the last file printed that gave the problem. But it's your choice. The code you had works.
python/lsst/obs/base/read_defects.py
Outdated
| dirs = [d for d in dirs if os.path.isdir(os.path.join(root, d))] | ||
| defects_by_chip = {} | ||
| name_map = {det.getName().lower(): det.getName() for | ||
| det in camera} # we assume the directories have been lowered |
There was a problem hiding this comment.
I find the comment cryptic. I assume you are saying you assume the directory names are lowercase, but it's not immediately clear how that relates to the code in question.
I suggest that instead of assuming directory names are lowercase, you do one of two things:
- Check that dir names are lowercase and raise if not. One logical place to do this is right after
dirs = os.listdir(root). - Replace
chip_name = name_map[os.path.basename(d)]withchip_name = name_map[os.path.basename(d).lower()]. This is simple but will not catch the case of two directories whose names only differs by case.
There was a problem hiding this comment.
The problem is that the names are not always lower case in the Camera so we have to map between on disk names and what the camera thinks. We wanted to assume the filesystem was lower to avoid issues with non-case sensitive file systems.
I've moved all the camera translation in to read_all_defects. Maybe that is clearer?
There was a problem hiding this comment.
I was suggesting that when you compare names you lowercase them (and I suggested two different ways to do that). I think that avoids the need for this dict and simplifies the code a bit. But your choice. The code is fine (or would be if the comment was clearer). I'll assume your new code is also fine.
SimonKrughoff
force-pushed
the
tickets/DM-18739
branch
from
0a55c53
to
c436213
Compare
| ------- | ||
| `lsst.afw.image.Exposure` or item | ||
| The standardized object. | ||
| """ |
|
The new doc strings look great! I see Travis is unhappy; I assume you'll deal with that before merging. |
SimonKrughoff
force-pushed
the
tickets/DM-18739
branch
3 times, most recently
from
e57dae5
to
4e120ee
Compare
Standard format is x0, y0, x_extent, y_extent.
Code was moved from here to the Defects factory method as part of DM-19373.
It turns out that not all calibration types will be Image-y. For example, defects should not have the sanitization method applied. This commit adds a try block and passes if sanitization fails with a TypeError.
Defects live in directories, so make sure the reference is a directory before attempting to traverse it.
SimonKrughoff
force-pushed
the
tickets/DM-18739
branch
from
4e120ee
to
47871be
Compare
No description provided.