DM-41049: Add matched color difference plots/metrics #156
Conversation
taranu
force-pushed
the
tickets/DM-41049
branch
from
0713951
to
4da8986
Compare
taranu
force-pushed
the
tickets/DM-41049
branch
from
4da8986
to
4ea7490
Compare
There was a problem hiding this comment.
There are lots of random little comments, but I am happy for you to use your judgement on most of this. The only real place I would like changes, or at least explanation is where you are introducing the dict types that need changes in pex_config. I think there are other ways of doing that as I mentioned and I would rather see them done that way instead of changing pex_config.
| color1_flux2 = ConfigurableActionField[VectorAction](doc="Action providing first color's second flux") | ||
| color2_flux1 = ConfigurableActionField[VectorAction](doc="Action providing second color's first flux") | ||
| color2_flux2 = ConfigurableActionField[VectorAction](doc="Action providing second color's second flux") | ||
| returnMillimags = Field[bool](doc="Use millimags or not?", default=True) |
There was a problem hiding this comment.
I think this doc string is weirdly worded as part statement part question
There was a problem hiding this comment.
It's copypasta from MagDiff which dates back to analysis_drp, but sure, now is as good a time as any to fix them.
|
|
||
| flux_err1 = ConfigurableActionField[VectorAction](doc="Action providing error for first flux") | ||
| flux_err2 = ConfigurableActionField[VectorAction](doc="Action providing error for second flux") | ||
| returnMillimags = Field[bool](doc="Use millimags or not?", default=True) |
| @@ -30,6 +32,11 @@ | |||
| class MagnitudeScatterPlot(MagnitudeXTool): | |||
| """A scatter plot with a magnitude on the x-axis.""" | |||
|
|
|||
| suffixes_y_finalize = ListField[str]( | |||
| doc="Suffixes for y-axis keys to finalize summary stats for", | |||
There was a problem hiding this comment.
No, that's it... but I'll re-word it.
| # ... but ScatterPlotStatsAction uses self.identity for names | ||
| # And then ScatterPlotWithTwoHists relies on self.identity | ||
| # being Stars/Galaxies | ||
| setattr(self.process.calculateActions, plural, statAction) |
There was a problem hiding this comment.
Arn’t you going to be overwriting whatever plural is each time through the loop?
There was a problem hiding this comment.
Whoops... good point.
| from .genericProduce import MagnitudeScatterPlot | ||
|
|
||
|
|
||
| class MatchedRefCoaddToolBase(MagnitudeXTool, ExtendednessTool): | ||
| class DictMetricAction(MetricAction): |
There was a problem hiding this comment.
I really don’t understand why you need a dictionary to store actions here, especially as it means you need to modify pex_config. Why not use ConfigurableActionStructFields? Those don’t required exact types, and they also give you the name associated with whatever the user assigned the action to. You can use .fieldNames to get the names for each Action in the struct, or use items if you want to iterate over the (name, action) pairs.
There was a problem hiding this comment.
In general do you even need this class at all?
| else: | ||
| y_any = np.concatenate([data[f"y{self._datatypes[key].suffix_xy}"] for key in self.plotTypes]) | ||
| y_any = ( |
|
|
||
| def getInputSchema(self) -> KeyedDataSchema: | ||
| # Something is wrong with the typing for DictField key iteration | ||
| schema = [] |
There was a problem hiding this comment.
is there any reason you wanted to store a list and extend vs. yield from ? It's not wrong, you don't need to change it, I was just curious if you chose it for a reason.
| actions = ConfigDictField[str, PlotAction](doc="Named plot actions") | ||
|
|
||
| def getInputSchema(self) -> KeyedDataSchema: | ||
| schema = [] |
There was a problem hiding this comment.
same here if you choose to change
| ), | ||
| ), | ||
|
|
||
| class MatchedRefCoaddDiffColorTool(MatchedRefCoaddDiffTool, MagnitudeScatterPlot): |
There was a problem hiding this comment.
is multiple inheritance here much better than construction with composition?
| default={"u": "g", "g": "r", "r": "i", "i": "z", "z": "y"}, | ||
| ) | ||
|
|
||
| def finalize(self): |
There was a problem hiding this comment.
When finalize functions start getting this extensive, I wonder if it is not better to just define your own process or produce actions, with their own __call__ and assign those.
taranu
force-pushed
the
tickets/DM-41049
branch
3 times, most recently
from
ef99026
to
56fa142
Compare
The min/max within a bin is only really useful if it's partially filled or otherwise limited, which won't be the case in most real usage.
Add a method that allows a PlotAction to specify what type name should be used when dynamically building plot output connection names.
No description provided.