DM-46342: Reorder and pad artifact mask handles #72
Conversation
| if dataIds != psfMatchedDataIds: | ||
| supplementaryData.warpRefList = reorderAndPadList( | ||
| supplementaryData.warpRefList, psfMatchedDataIds, dataIds | ||
| ) |
There was a problem hiding this comment.
I have no idea what I was thinking here. These are not used in this block, and supplementaryData.warpRefList is already reordered and padded by the time it gets here.
There was a problem hiding this comment.
And those pads were the Nones that were causing the crashes as I understand it?
There was a problem hiding this comment.
They were Nones yes. But the supplementaryData was never used in this block or after, making these lines unnecessary.
| if dataIds != psfMatchedDataIds: | ||
| supplementaryData.warpRefList = reorderAndPadList( | ||
| supplementaryData.warpRefList, psfMatchedDataIds, dataIds | ||
| ) |
There was a problem hiding this comment.
And those pads were the Nones that were causing the crashes as I understand it?
| ): | ||
| if warpRef is None: | ||
| continue | ||
| assert warpRef.dataId == outputRef.dataId, "DataId mismatch between warp and artifactMask" |
There was a problem hiding this comment.
This I don't understand. Isn't it possible for the artifactMaskRefList to be padded with None and then outputRef.dataId isn't valid? I would think that this should have an if outputRef is None: continue to account for the possible padding.
There was a problem hiding this comment.
You're absolutely right. I should condition on outputRef.
There was a problem hiding this comment.
I'm back to saying that checking for None and continueing is unnecessary.
First, these data handles are from the time QG was generated and they know nothing about which quanta actually succeeded. This is particularly true with outputRef, which would only get reordered and never padded. Second, the altMaskList may be padded, and yet a check is not warranted. If the padded value (it's a dict with specific schema of empty list) is passed, it simply produces a trivial mask with all 0s (these are just the additional mask that needs to be applied). It'd be cleaner to persist it than skip it, since the storage cost to save a mask is low and similar masks would be persisted if there were no artifacts whatsoever. If we attempt to not persist this for padded quanta, then we would have to special case in assemble_cell_coadd and it would not be worth the trouble IMO.
P.S.: I was concerned why the previously failing quanta were passing with this branch setup even before I added this guard, and I have an explanation now.
There was a problem hiding this comment.
Ah I see your point. All output refs will be defined, so there won't be padding there. That makes sense.
There was a problem hiding this comment.
For the future us, if the corresponding PSF-matched warp is missing for a warp, then the artifact_mask is set to NO_DATA everywhere, which will automatically avoid a warp without its PSF-match counterpart from entering into a coadd.
arunkannawadi
force-pushed
the
tickets/DM-46342
branch
from
2033287 to
afcb1ef
Compare
No description provided.