-
Notifications
You must be signed in to change notification settings - Fork 2
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-43878: Supporting multiple plotted quantities in CalibAmpScatterTool #265
base: main
Are you sure you want to change the base?
Conversation
3d50402
to
6593043
Compare
This commit changes the `quantityKey` field in the `CalibAmpScatterTool` class to be of type `ListField`. This enables multiple quantities to be plotted on a single scatter plot. In addition, pipeline YAML files for calibration verification are re-worded to use the new `ListField` type
b57cba3
to
9078f61
Compare
this now works and I've verified that the plots produced are identical to the original. Should we wish to change some plots to have multiple series in the calibAmpScatterTool it can now be done with the new ListField for quantityKey |
Can I ask that this be renamed according to the style guide (https://developer.lsst.io/work/flow.html#make-a-pull-request)? I'm also somewhat hopeful that this ticket will magically fix another problem I'm having with these plot types, so I'm sorry it's taken so long to get to the review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let @natelust say if he has any concerns here, but this seems like an elegant fix to me.
doc="Figure size.", | ||
default=[8, 8], | ||
) | ||
figsize = ListField[float](doc="Figure size.", default=[8, 8], length=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be expanded out to match the other fields formatting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@czwa I'm not sure I understand, do you just mean the code formatting? Or to change the other fields to further constrain their formatting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just code formatting, and I prefer the expanded "one element per line" version.
I think this has been open long enough, so ping me after you've rebased, I'll do one final quick scan, and we can move ahead with getting this merged (without waiting for @natelust ). |
This is roughly how I'm thinking to implement the ability to plot multiple data series in CalibAmpScatterTool, and how the updated pipelines would look. It doesn't quite work yet, a couple more commits to come.