-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-49274: Stop sky objects appearing in NO_DATA regions #1056
Conversation
c991c8b
to
820103b
Compare
cc7fde0
to
e4fb626
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great to split commit acee1a3 into two - one rearranging the names and one adding the nodata flag, but I am not insisting on it.
- base_PixelFlags_flag_interpolatedCenter | ||
- base_PixelFlags_flag_nodata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add base_PixelFlags_flag_nodataCenter
and then @jrmullaney won't have to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest doing this in all the schemas where other flags have "center" variants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure adding it in this block will work. Aren't these pulling flags from the calexp, rather than the diff? My understanding is that the nodata flag that I want in the forcedSourceTable comes from the diff catalog, not the calexp. See here.
Having said that, if @leeskelvin wants to add that in a block above (as in that link-to PR), then I could delete my PR and have it all done in one go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is probably true, but I still think we should add it here! I forgot this schema was for the merged forcedSource catalog, and not for the one upstream a step or two before the merge happens, which doesn't care what flavor of exposure it is run on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind adding it here, and the 4 other schema files at the same time. Troubled by this though:
I'm not sure adding it in this block will work.
If I add *nodataCenter
into all 5 schemas, will something break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That feels like a "be sure to run ci_*" flavored question 😅
When @jrmullaney said he wasn't sure it would work, I think he meant it might not let him do the downstream selection/filtering on merged forced source tables to the extent we want, not that it would break anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added base_PixelFlags_flag_nodataCenter
to the five schema YAMLs in this repo. Currently running Jenkins with:
lsst_distrib lsst_ci lsst_middleware lsst_sitcom ci_cpp ci_hsc ci_imsim ci_middleware
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussion with @jrmullaney, I've removed this column. The column needed is different than a straight-forward propagation, and requires more thought and testing. @jrmullaney will follow this up on DM-49554.
e4fb626
to
97f4b7c
Compare
It wouldn't be an @arunkannawadi review without at least one request to split a commit into multiple commits 😉. Commit split! Just waiting on a reply from @jrmullaney / @mrawls Re: |
31270c7
to
97f4b7c
Compare
No description provided.