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-28334: Replace afw.image.Filter name usage with explicit map. #412

Merged
merged 1 commit into from Mar 4, 2022

Conversation

TallJimbo
Copy link
Member

We have a few data files committed to git here that use various old names for filters. It's best to just have an explicit constant mapping for those.

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 think my only real concern is whether this code is actually run anywhere, since I thought the transmission curves that we use are pre-built and just exist in obs_subaru_data?

value from the short one (e.g. 'r') declared canonical in afw.image.Filter.

def getPhysicalFilterName(short):
"""Return am HSC physical_filter name (e.g. 'HSC-R') that's usable as a
Copy link
Contributor

Choose a reason for hiding this comment

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

"am"->"an"

if short == filter.afw_name or short == filter.band:
return filter.physical_filter
return short
if short.startswith("N"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth a comment above this that it's just here for the narrowband filters?

@@ -36,20 +35,23 @@

HSC_BEGIN = "2012-12-18" # initial date for curves valid for entire lifetime of HSC

OLD_FILTER_NAME_MAP = {b: f"HSC-{b.upper()}" for b in "grizy"}
OLD_FILTER_NAME_MAP.update(r2="HSC-R2", i2="HSC-I2")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a comment above this saying something like "this is to map physical filter names to the deprecated afw names", for future readers?

@timj
Copy link
Member

timj commented Mar 3, 2022

Also:

we have a few data files committed to git here

Can't we fix the files?

@TallJimbo
Copy link
Member Author

I think my only real concern is whether this code is actually run anywhere, since I thought the transmission curves that we use are pre-built and just exist in obs_subaru_data?

Yes, these are still run: HyperSuprimeCam.writeAdditionalCuratedCalibrations calls the functions in makeTransmissionCurves.py to assemble TransmissionCurve instances from the data files in obs_subaru/hsc/transmission, and then writes those to the data repository.

Also:

we have a few data files committed to git here

Can't we fix the files?

I just don't think it's worth it.

We have a few data files committed to git here that use various old
names for filters.  It's best to just have an explicit constant mapping
for those.
@TallJimbo TallJimbo merged commit 841ccd4 into main Mar 4, 2022
@TallJimbo TallJimbo deleted the tickets/DM-28334 branch March 4, 2022 16:41
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

3 participants