DM-38004: Fix metrics printing in histPlot #70
Conversation
psferguson
force-pushed
the
tickets/DM-38004
branch
from
f2f7b7d
to
c78393f
Compare
| default_stats_panel_bool = self.panels[panel].statsPanel.stat1 is None | ||
| default_stats_panel_bool &= self.panels[panel].statsPanel.stat2 is None | ||
| default_stats_panel_bool &= self.panels[panel].statsPanel.stat3 is None | ||
|
|
There was a problem hiding this comment.
So what happens if you set, say, stat2, but not stat1 or stat3? If it must be all or none, the config settings should be validated.
There was a problem hiding this comment.
Ok, I think I have setup config validation correctly, so that if must be all or none.
I also set the task to fail if somehow a incorrectly configured panel makes it to the _makePanel method
| @@ -429,14 +443,21 @@ def _makePanel(self, data, panel, ax, colors, label_font_size=9, legend_font_siz | |||
| if self.panels[panel].referenceValue is not None: | |||
| ax = self._addReferenceLines(ax, panel, panel_range, legend_font_size=legend_font_size) | |||
|
|
|||
| if self.panels[panel].statsPanel is None: | |||
| # check if we should use the default stats panel or if a custom one | |||
There was a problem hiding this comment.
The first word should be capitalized.
| raise FieldValidationError(self.__class__.stat2, self, msg) | ||
| if not bool(self.stat3): | ||
| raise FieldValidationError(self.__class__.stat3, self, msg) | ||
|
|
There was a problem hiding this comment.
With this raise structure, it seems we only get to know about the first offending entry, so, e.g. if one tried to specify only stat2, we would learn that step1 wasn't set but would get no warning about step3? Also, the need for all three should also be made clear in the config parameter docs as well (with the hopes that if the reader reads them, they would not encounter this validation error). Speaking of those docs, I find the wording confusing as I don't know what "the first/second/third scalar statistics" are/are meant to be (and why fixed at three?)
There was a problem hiding this comment.
Correct, I had hoped the error message when a validation fails If one stat is configured, all 3 stats must be configured, would be enough, but I am open to suggestion on how to improve this. Would you recommend checking them all and setting a flag if they fail, and then returning an error that specifies which ones fail.
They are fixed at 3 because of how the statsPanel was implemented in hist plot:
analysis_tools/python/lsst/analysis/tools/actions/plot/histPlot.py
Lines 583 to 591 in 89b75f2
I think format was chosen to get the right spacing and behavior for the 3 column default case, but breaks if more or fewer columns are provided, and does not easily generalize.
I called them stat1/2/3 because they are supposed to be very general, I wanted to make an easy way showing metrics (such as PA1 and PF1) on a hist plot. I also wanted to make sure the metric calculation used the same code as the stat panel in the histogram; this is why the stats are computed as a build action and passed to hist plot for the produce stage. I am open to calling them something else, but am not sure what would be better.
There was a problem hiding this comment.
I leave this all up to you (i.e. I don't really have better suggestions that wouldn't involve a level of verbosity along the lines of your explanations above that some may not like...)! Personally, I'd prefer just to get the message that If one stat is configured, all 3 stats must be configured with no attempt to identify which ones are invalid as opposed to a partial (and thus potentially misleading) list (i.e. specify all or none).
There was a problem hiding this comment.
Unfortunately pex.config.FieldValidationError requires a specific field to be raised (and can only mention 1 field), so I implemented a ValueError in the validation portion with a generic message example here:
ValueError: plots.skySourceHistPlot.produce.panels['panel_sn'].statsPanel : If one stat is configured, all 3 stats must be configured
psferguson
force-pushed
the
tickets/DM-38004
branch
from
89b75f2
to
07d4824
Compare
fix behavior where default stats are not shown