tickets/DM-16558: removeMaskPlane function in multiband.py does not work #416
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
laurenam
force-pushed
the
tickets/DM-16558
branch
from
7aaa884
to
029b214
Compare
isullivan
approved these changes
Nov 24, 2018
| """ | ||
| # Clear all masks in MultibandMask but leave in default dict for now | ||
| for single in self.singles: | ||
| single.removeAndClearMaskPlane(name, False) |
There was a problem hiding this comment.
I think single.removeAndClearMaskPlane(name, removeFromDefault=False) would be more clear.
| for single in self.singles: | ||
| single.removeAndClearMaskPlane(name, False) | ||
| # Now remove from default dict according to removeFromDefault | ||
| self._refMask.removeAndClearMaskPlane(name, removeFromDefault) |
There was a problem hiding this comment.
Would self.Mask().removeAndClearMaskPlane(name, removeFromDefault) work?
There was a problem hiding this comment.
I’m not entirely sure, but I think not as all the non-static methods this class do it this way (e.g. clearAllMaskPlanes, getMaskPlaneDict, getNumPlanesUsed, etc...), and removeAndClearMaskPlane is a non-static method:
https://github.com/lsst/afw/blob/master/include/lsst/afw/image/Mask.h#L477
There was a problem hiding this comment.
To be sure, I ran this by @TallJimbo and he concurred.
The removeMaskPlane was broken in two ways: 1) the removeMaskPlaneDict() function called is not an attribute of the mask being updated, so any call to MultibandMask's removeMaskPlane() results in an AttributeError. The fix is to update the function name from removeMaskPlaneDict() to removeMaskPlane(). 2) the function ignores the variable "name", i.e. does not pass it to the call to [sic!] removeMaskPlaneDict(). Having corrected the function name to removeMaskPlane(), we then get a TypeError. The fix here is to pass name into removeMaskPlane(name). These issues were not caught because removeMaskPlane() was not being exercised in any unittest, so one is added here.
This function was missing in the MultibandMask class. A unittest was also added.
laurenam
force-pushed
the
tickets/DM-16558
branch
from
029b214
to
e9b0058
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.