DM-36231: Factor out duplicate code between stellar locus plots and metrics. #40
Conversation
sr525
force-pushed
the
tickets/DM-36231
branch
3 times, most recently
from
7e37084
to
abddc9d
Compare
There was a problem hiding this comment.
I confirmed that it runs and produces plots/metrics, but only on rc2_subset, so the outputs aren't that meaningful.
Since the output of all the stellar locus metrics (and summary values on plots) should be in mmag, it would be good to implement that conversion on this branch before pushing. Otherwise, this refactor has made things much more readable and useable.
Good to go once you've addressed the few comments I added!
| } | ||
|
|
||
| # self.process.calculateActions.xPerp = StellarLocusFitAction() | ||
| # self.process.calculateActions.xPerp.stellarLocusFitDict = {} | ||
|
|
||
| self.produce.units = { # type: ignore | ||
| "wPerp_psfFlux_sigmaMAD": "mag", # TODO need to return mmag from wPerp |
There was a problem hiding this comment.
If we want this to be mmag, is there a reason why we're not doing that by default? (Same question applies for all other stellar locus classes.)
There was a problem hiding this comment.
No idea, that wasn't my comment, I can look into changing it if we want to?
There was a problem hiding this comment.
I think we should change it. In both the plots and the metric, we want stellar locus widths in mmag.
| def setDefaults(self): | ||
| super().setDefaults() | ||
| self.prep.selectors.flagSelector.bands = ["g", "r", "i"] | ||
| self.prep.selectors.snSelector.bands = ["r"] |
There was a problem hiding this comment.
Two questions:
- In the analysis_drp version, I think the default S/N band was "i" -- do we want to retain that?
- Wouldn't it make sense to apply a S/N cut in all three bands that go into this plot/metric?
There was a problem hiding this comment.
I think I like the choice of r-band here, since it is part of the colors on both axes, but it's worth making sure that's what we want.
There was a problem hiding this comment.
It is probably another ticket to discuss/check/change these things.
There was a problem hiding this comment.
Sure, we can save it for another ticket. But we should probably settle on a "standard" for DM before thousands of metrics start appearing in analysis_tools.
| self.prep.selectors.snSelector.bands = ["r"] | ||
| self.prep.selectors.snSelector.fluxType = "{band}_psfFlux" | ||
|
|
||
| self.prep.selectors.starSelector.vectorKey = "r_extendedness" |
There was a problem hiding this comment.
Similar to above comment -- do we want r-band as the default for extendedness? The seeing will typically be better in i-band...
Maybe this is a larger conversation that should be had regarding "defaults" in QA work.
|
Another thing that occurred to me after reviewing: we should persist more than just the sigmaMAD of each stellar locus metric. Might be good to keep some other stats, and probably most importantly to keep the fit parameters so plots/metrics can be recreated. |
yalsayyad
changed the title
|
I changed the units to mmags and added returning the median as well. How do we want to propagate the fit parameters? |
sr525
force-pushed
the
tickets/DM-36231
branch
from
abddc9d
to
121a6f1
Compare
sr525
force-pushed
the
tickets/DM-36231
branch
5 times, most recently
from
342d9ae
to
4d9ad93
Compare
sr525
force-pushed
the
tickets/DM-36231
branch
from
4d9ad93
to
ec9ab76
Compare
No description provided.