DM-34124: Add separate histogram label argument into histPlot #29
Conversation
There was a problem hiding this comment.
Looks fine for this change set, with some minor tweaks requested.
Outside the scope of this ticket: This function is really huge and complex with lots of specific syntax baked in.
I feel it would read better if it was abstracted away into other methods or free functions such that the main body of the method is 5-15 lines of code dispatching to the other functions with names describing what they are doing. Potentially those methods too might need abstracted a bit as well.
I think that would leave things in a much more maintainable state, that is easier for others to understand and update. Again, not important for this ticket, just a general comment if you happen to have a good point to consider it.
python/lsst/analysis/drp/histPlot.py
Outdated
| @@ -46,8 +46,16 @@ class HistPlotConfig(pexConfig.Config): | |||
| ) | |||
|
|
|||
| actions = ConfigurableActionStructField( | |||
| doc="A dict of configurable actions, with each key-value pair corresponding to each histogram. " | |||
| "Dict keys will be used to label each histogram in the panel.", | |||
| doc="A dict of configurable actions, with each item corresponding to a single histogram.", | |||
There was a problem hiding this comment.
ConfigurableActionStructField is NOT a dict, it is like a Struct or SimpleNamespace object. It only supports dict like assignment and construction in order to save developers the trouble of typing out config.actions.x = 1; config.actions.y = 2, ... everywhere in a similar way to initializing the above mentioned classes with kw args.
There was a problem hiding this comment.
Thanks. Changed to:
A Struct-like object, with each item corresponding to a single histogram action.
python/lsst/analysis/drp/histPlot.py
Outdated
|
|
||
| histLabels = pexConfig.DictField( | ||
| doc="A dict specifying the histogram labels to be printed in the upper corner of each panel. If the " | ||
| "key matches a key specified in `actions`, then the corresponding value will be printed on the plot. " |
There was a problem hiding this comment.
| "key matches a key specified in `actions`, then the corresponding value will be printed on the plot. " | |
| "matches an `attribute` of the `actions` field, ..." |
There was a problem hiding this comment.
Thanks, changed as suggested!
leeskelvin
force-pushed
the
tickets/DM-34124
branch
from
790dc40
to
2cdcf41
Compare
No description provided.