DM-29583: Create dataset class for extended PSF models #495
Conversation
There was a problem hiding this comment.
I haven't looked at the test file carefully enough yet, and I'll go over them once you address these comments and rebase it. Also, the filename in snake case stands out oddly when compared to other files, but I guess that's okay given the recent discussions about naming of files in Slack?
MorganSchmitz
force-pushed
the
tickets/DM-29583
branch
from
22c5d30
to
46cccec
Compare
|
Thanks a lot for all the comments, Arun. It should all be ready for your eyes again. I agree the snake_case filename stands out for now, but I think that's just part of our slowly transitioning to mostly-snake_case...? ¯_(ツ)_/¯ |
| if has_default and md["REGION"] == "DEFAULT": | ||
| if md["EXTNAME"] == "IMAGE": | ||
| default_image = afwImage.ImageF(filename, hdu=j) | ||
| elif md["EXTNAME"] == "MASK": |
There was a problem hiding this comment.
Could you add a small test to check this case?
There was a problem hiding this comment.
Huh. This should be covered by this test (both image and mask are always saved).
Did it show as uncovered by coverage.py?
There was a problem hiding this comment.
Hmm, this is what it says as missing: 66-68, 83-89, 112-118, 129-132, 154-158, 169-195, 200, 212-250, 256. Most of them are where you raise an error.
|
I haven't looked into the test file in as much detail as the other one, except that it covers all the line. I made a few minor comments about what is not covered by the unittest, but that's me testing out the tool |
|
And oh, reminder to fixup your review round 1 commit with the other ones. |
|
Will do! I like to leave it during the review process so the referee can get GitHub's diff to show changes before everything gets squashed together. Thanks tons, Arun! |
MorganSchmitz
force-pushed
the
tickets/DM-29583
branch
from
46cccec
to
288a75b
Compare
Main PR for DM-29583, the other being in daf_butler.
Jenkins run #33904 contains these changes, plus those of DM-25305 (ticket was split in two).