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-34692: Fix band list for CoaddPlotFlag & Sn Selectors in coaddQAEllip pipeline #40

Merged
merged 2 commits into from May 10, 2022

Conversation

laurenam
Copy link
Contributor

@laurenam laurenam commented May 7, 2022

No description provided.

The default for the bands list is already ["i"] for the SnSelector,
so it should not be included here.  On the other hand, the default
for the CoaddPlotFlagSelector is ["g", "r", "i", "z", "y"] and the
plots will end up empty if not all of these bands are present (even
though only the i band is being plotted), so these do need to be
overridden here.  This was noticed when running ci_hsc_gen3 which
only includes r and i band data, so plots for this pipeline were all
empty.
Added missing entry to __all__ and reordered entries to match
their order of appearance in the file.
Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -1,5 +1,6 @@
__all__ = ("FlagSelector", "PsfFlagSelector", "StarIdentifier", "GalaxyIdentifier", "UnknownIdentifier",
"CoaddPlotFlagSelector", "BaseSNRSelector", "SnSelector")
__all__ = ("FlagSelector", "PsfFlagSelector", "BaseSNRSelector", "SnSelector",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure about exposing BaseSNRSelector here. It doesn't seem to be used anywhere(?) and if it is used, it would be to define other SNR selectors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that it is unused too and plan to address that on another ticket (I first want to check with the original author that removing it is the right thing to do). Since it was already in the list, I'll leave it as is here and deal with it on the removal ticket.

@laurenam laurenam merged commit 017d932 into main May 10, 2022
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