Skip to content
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-37386: Add a CenterAll flag during detect and measure #260

Merged
merged 3 commits into from Nov 27, 2023

Conversation

parejkoj
Copy link
Contributor

No description provided.

Copy link
Contributor

@fred3m fred3m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it took me a little while but I think I understand how this works now. I think that the use of _bits and _bitsAll is confusing, since _bitsAll doesn't really mean all of the bits in the footprint had a given masked bit set, just the center 3x3 grid, and when paired with _bits it's more confusing.

I understand what you were trying to convey, since _bits is the union of all of the bits in the spanset and _bitsAll is the intersection of all of the bits in the spanset, so I would recommend renaming them accordingly, ie. _unionBits and _intersectionBits, or _anyBit and _allBits.

Comment on lines +127 to +128
_centerKeys["BAD"] = schema.addField<afw::table::Flag>(name + "_flag_badCenter",
"Bad pixel in the 3x3 region around the centroid");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this is formatted differently than all of the other schema fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting is entirely determined by clang-format: in this case, I think it's because the docstring is shorter.

Rename bits in file-local FootprintBits to distinguish the two kinds.
@parejkoj parejkoj merged commit 1ce4c50 into main Nov 27, 2023
2 checks passed
@parejkoj parejkoj deleted the tickets/DM-37386 branch November 27, 2023 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants