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.

Sign up for GitHub

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

Jump to bottom

DM-26007: defaultFilter is not used if a filterName is given to loadSkyCircle #163

Merged
merged 3 commits into from Aug 4, 2020
Merged

DM-26007: defaultFilter is not used if a filterName is given to loadSkyCircle #163

merged 3 commits into from Aug 4, 2020

Conversation

parejkoj
Copy link
Collaborator

No description provided.

r-owen
r-owen approved these changes Jul 20, 2020
View reviewed changes
Copy link
Contributor

@r-owen r-owen 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 a bit surprised that the old filter map mapped everything to "g", but assuming there was a good reason for it, this seems a reasonable change.

If there really is only one reasonable astrometric reference filter, why not leave defaultFilter blank?

@parejkoj
Copy link
Collaborator Author

"Why not leave defaultFilter blank?" - I don't understand this statement. I thought this was exactly the use case for defaultFilter.

Jointcal needs some kind of mapping between camera and reference filters because loadSkyCircle has to pick a filter (which jointcal then uses internally for things). I'm using defaultFilter here so that when I load the refcat, I can always just supply the camera filter jointcal is using, without knowing anything about the configuration of the refcat. Gaia DR2 comes with three filters (G, which is not at all SDSS g but rather is very broad, BP and RP), so we have to specify at least a defaultFilter.

@r-owen
Copy link
Contributor

r-owen commented Jul 23, 2020

Looking at the code in your other pull request it appeared that defaultFilter=None would pick camFlux. Since that is the filter your new code falls back to, it looked like leaving defaultFilter None would be a simpler solution.

@parejkoj
Copy link
Collaborator Author

camFlux gets defined when you set defaultFilter, that's the only way it gets defined.

@parejkoj parejkoj force-pushed the tickets/DM-26007 branch 2 times, most recently from 635b4c7 to d8a8192 Compare July 29, 2020 17:36
r-owen
r-owen approved these changes Aug 2, 2020
View reviewed changes
Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

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

Looks good. Another slightly surprising instance of setting the trivial filter map to None, when that is already the default.

tests/config/astrometryReferenceErr-None-config.py Show resolved Hide resolved
tests/config/hsc-gaia-config.py Show resolved Hide resolved
tests/config/astrometryReferenceErr-None-config.py Outdated Show resolved Hide resolved
@parejkoj
Make jointcal use new anyFilterMapsToThis
14b508f 
The intent of the previous filterMap settings was the same, but this is
much simpler with the new option.

HSC tests need explicit overrides until DM-25849 is completed.
@parejkoj
remove deprecated HSC config file
fb152e1
@parejkoj
fix test name to match others
d29832e
@parejkoj parejkoj merged commit b0260a2 into master Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r-owen r-owen r-owen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@parejkoj @r-owen