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-28597: Fix colorterm/photoCal filterLabel confusion #55

Merged
merged 3 commits into from Mar 31, 2021

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Mar 10, 2021

No description provided.

Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

I'm concerned about the "reverse lookup" of band->physical, which is definitely not one-to-one.

@@ -745,7 +747,7 @@ def generateTractOutputProducts(self, dataRefDict, tract,

return retStruct

def _computeReferenceOffsets(self, stdCat, bands):
def _computeReferenceOffsets(self, stdCat, lutCat, physicalFilterMap, bands):
"""
Compute offsets relative to a reference catalog.
Copy link
Contributor

Choose a reason for hiding this comment

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

When we chatted, you mentioned that this was mostly used for testing. Should that be made clear in the docs? Because I worry that the implementation may now break if someone tries to use it on data that has multiple physical filters mapping to the same band.

# Find a physical filter associated from the band by doing
# a reverse lookup on the physicalFilterMap dict
physicalFilterMapIndex = physicalFilterMapBands.index(band)
physicalFilter = physicalFilterMapFilters[physicalFilterMapIndex]
Copy link
Contributor

Choose a reason for hiding this comment

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

It probably doesn't matter for the tests this is used in, but I suspect this will not give the results we want for e.g. HSC data that has both HSC-I and HSC-I2, because those are both band="i".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way the lookup works is that we go from band to a physical filter and then to the standard filter. In the case that we have both HSC-I and HSC-I2, these two physical filters will be mapped to the same standard filter (the default is HSC-I2). This is enforced in the fgcm code that that you can't have a band mapped to more than one standard filter. Therefore, we get i to HSC-I to HSC-I2 or i to HSC-I2 to HSC-I2 but no matter the lookup path you end up with the same standard filter, and the one that was specified should be used (and thus get the correct color terms). I'll add more comments in the code.

@parejkoj parejkoj merged commit 1721dae into master Mar 31, 2021
@parejkoj parejkoj deleted the tickets/DM-28597 branch March 31, 2021 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants