Fix _apply_sky in skymatch for single group input #5440
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5440 +/- ##
==========================================
- Coverage 71.72% 71.08% -0.64%
==========================================
Files 414 414
Lines 37755 38170 +415
Branches 5843 6029 +186
==========================================
+ Hits 27079 27133 +54
- Misses 8956 9310 +354
- Partials 1720 1727 +7
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report at Codecov.
|
jwst/skymatch/skymatch.py
Outdated
| log.warning(" * {:s} ID={}: Unable to compute sky value" | ||
| .format(img_type, img.id)) | ||
| .format(img_type[is_group], img.id)) |
There was a problem hiding this comment.
Is it kosher to use a boolean (is_group) as an index into a list? I guess it sort of works by having False and True equate to 0 and 1, but ...
There was a problem hiding this comment.
Is there anything kosher in Python? Unfortunately not a Python priest here. Although, booleans are a subclass of int and so I do not think there is anything wrong with this.
There was a problem hiding this comment.
Possible alternatives:
img_type[int(is_group)]
or
img_type[is_group + 0]
I like this last one above :)
Other alternatives (both overkill IMO): make img_type a dictionary or have an if statement.
There was a problem hiding this comment.
I found something: https://www.python.org/dev/peps/pep-0285/ - see point 6:
Should bool inherit from int?
=> Yes.
In an ideal world, bool might be better implemented as a separate integer type that knows how to perform mixed-mode arithmetic. However, inheriting bool from int eases the implementation enormously (in part since all C code that calls PyInt_Check() will continue to work -- this returns true for subclasses of int). Also, I believe this is right in terms of substitutability: code that requires an int can be fed a bool and it will behave the same as 0 or 1.
There was a problem hiding this comment.
You like the last one above, because it's clever (adding an int value will cause the result to be int), while I like the first one (explicitly casting the result to int), because it's explicit (and hence doesn't require a reader to think about what's happening).
There was a problem hiding this comment.
Ha! priest has spoken (see same PEP, "Compatibility" section):
Because of backwards compatibility, the bool type lacks many properties that some would like to see. For example, arithmetic operations with one or two bool arguments is allowed, treating False as 0 and True as 1. Also, a bool may be used as a sequence index.
I don't see this as a problem, and I don't want evolve the language in this direction either. I don't believe that a stricter interpretation of "Booleanness" makes the language any clearer.
jwst/skymatch/skymatch.py
Outdated
| @@ -382,24 +380,26 @@ def match(images, skymethod='global+match', match_down=True, subtract=False): | |||
|
|
|||
|
|
|||
| def _apply_sky(images, sky_deltas, do_global, do_skysub, show_old): | |||
| img_type = ['Image', 'Group'] | |||
There was a problem hiding this comment.
I would add a comment here,
# The following to be indexed by boolean is_group
img_type = ['Image', 'Group']to make this clear.
Fixes a bug in
skymatch.skymatch._apply_sky()that would result in a crash whenskymethodcontains 'global' and input image is a single image group whose sky cannot be computed, e.g., because its images contains no "good" pixels.This bug was revealed after PR #5423 which permitted processing of single image/group inputs.
Fixes failing test
test_nircam_image.py::test_image3_closedfile. The test will still be failing for two reasons: