DM-27843: Add anyFilterMapsToThis support to ReferenceObjectLoader #274
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
parejkoj
force-pushed
the
tickets/DM-27843
branch
2 times, most recently
from
7c4b838
to
5c88bc0
Compare
TallJimbo
approved these changes
Dec 29, 2021
There was a problem hiding this comment.
I reviewed commit-by-commit, and some code moved around, so it's possible the locations some changes should be made have moved. But all comments are minor, and (I think) entirely about docs.
Overall, looks like a very nice cleanup. I think this doesn't actually touch the logic used to do the padding when actually loading refcat rows in regions, which is good - that code is super fragile and not particularly well tested in CI, IIRC, and there are subtle differences between how Gen2 and Gen3 work that I think are important. Someday we should try to fix that, of course, but for now I just want to make sure we avoid letting that problem torpedo unrelated "nearby" tickets like this one.
parejkoj
force-pushed
the
tickets/DM-27843
branch
from
5c88bc0
to
1107a06
Compare
This is the new base class that all gen2/gen3 loaders derive from, so we should use it in tests of loaders. This will let us move more methods into this base class.
* Use getMetadataCircle in getMetadataBox base method, but include the box corners additions to it from the gen3 code. * Move parameter docs for gen3 loader out of __init__. * Make _calculateCircle a static method, as it only needs pixelMargin. * Move makeMinimalSchema from gen2 base class, as we need it for making new catalogs no matter what. * Use ReferenceObjectLoaderBase in places that call static methods. s joinMatchListWithCatalog was identical between gen2 and gen3.
* Unify the flux field remapping and centroid adding, so we only need to call the _remap method, not both. * We only need to call this once per refcat load (gen3 loader was potentially calling it multiple times). * Use the new _remap method in gen2 LoadIndexedReferenceObjectsTask.
This is never called in the same context as the gen2 version, so we can remove it as it has been deprecated for years.
The only places that passed this arg were in meas_astrom, and I'm taking care of them on this ticket.
parejkoj
force-pushed
the
tickets/DM-27843
branch
from
1107a06
to
48cdb78
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.
As part of getting the gen3 code to support the more complicated filter mapping logic that the gen2 code did, I also refactored several methods into the common base class. Gen2 is going away soon, but we also want to keep as much of the gen2 test coverage as we can: this should help with that.