-
Notifications
You must be signed in to change notification settings - Fork 30
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
Ensure custom plots legend matches plot values #4729
Conversation
extension/src/plots/model/collect.ts
Outdated
@@ -93,6 +114,7 @@ const getCustomPlotData = ( | |||
const paramPath = getFullValuePath(ColumnType.PARAMS, param) | |||
|
|||
const values = getValues(experiments, metricPath, paramPath, renderLastIds) | |||
const filteredColorScale = filterColorScale(completeColorScale, values) |
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.
[Q] Is there a reason to do this after collecting completeColorScale
? Would be good to have all of the transformation/collection in the same place.
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.
Originally, I was trying to avoid looping over all the experiments for each plot, but I now see we're still looping over completeColorScale
anyway 😄. I'll update the code to create a single color scale per plot.
extension/src/plots/model/collect.ts
Outdated
colorScale: ColorScale | undefined, | ||
valueIds: string[] | ||
) => { | ||
const completeColorScale = filterColorScale(colorScale, valueIds) |
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.
We need to filter through the original color scale values first so they retain their order in the legend. Maybe there's a simpler way of doing this though 🤔
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.
something like
const removeSelectedExperiment = (
orderedColorScale: ColorScale,
hasValues: boolean,
idx: number
) => {
const isSelectedExperiment = idx !== -1
if (!isSelectedExperiment || hasValues) {
return
}
orderedColorScale.domain.splice(idx, 1)
orderedColorScale.range.splice(idx, 1)
}
const fillColorScale = (
experiments: Experiment[],
colorScale: ColorScale | undefined,
valueIds: string[]
) => {
const orderedColorScale = {
domain: [...(colorScale?.domain || [])],
range: [...(colorScale?.range || [])]
}
for (const experiment of experiments) {
const { id } = experiment
const idx = orderedColorScale.domain.indexOf(id)
const isSelectedExperiment = idx !== -1
const hasValues = valueIds.includes(id)
if (!hasValues || isSelectedExperiment) {
removeSelectedExperiment(orderedColorScale, hasValues, idx)
continue
}
orderedColorScale.domain.push(id)
orderedColorScale.range.push('#4c78a8' as Color)
}
return orderedColorScale
}
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.
Works great, thanks!
Code Climate has analyzed commit 12739c0 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.0% (0.0% change). View more on Code Climate. |
main
PR
Related to #4725