Skip to content
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.

Sign up for GitHub

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

Jump to bottom

5059 bug unable to see intervention in output chart for calibrate #5110

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+37 −5
Open

5059 bug unable to see intervention in output chart for calibrate #5110

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bb2b5e3
show intervention marker on chart, improve label position
mloppie Oct 9, 2024
7b5e1b7
show intervention on calibrate node graph, improve label position
mloppie Oct 9, 2024
63ca70f
lint
mloppie Oct 9, 2024
fbf44cb
Merge branch 'main' into 5059-bug-unable-to-see-intervention-in-outpu…
mloppie Oct 9, 2024
8e4f4d5
Merge branch 'main' into 5059-bug-unable-to-see-intervention-in-outpu…
mloppie Oct 11, 2024
952fcd0
Merge branch 'main' into 5059-bug-unable-to-see-intervention-in-outpu…
mloppie Oct 16, 2024
364cd67
add intervention charts to calibrate dd
mloppie Oct 16, 2024
f1b56b8
Merge branch 'main' into 5059-bug-unable-to-see-intervention-in-outpu…
mloppie Oct 16, 2024
38dba26
Merge branch 'main' into 5059-bug-unable-to-see-intervention-in-outpu…
mloppie Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ...i-client/src/components/workflow/ops/calibrate-ciemss/tera-calibrate-ciemss-drilldown.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,7 +783,11 @@ const preparedCharts = computed(() => {
annotations
);

charts[variable].layer.push(...createInterventionChartMarkers(groupedInterventionOutputs.value[variable]));
Object.entries(groupedInterventionOutputs.value).forEach((item) => {
item[1].forEach((intervention) => {
charts[variable].layer.push(...createInterventionChartMarkers([intervention], false, -180));
});
});
});
Copy link
Contributor

@jryu01 jryu01 Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we are applying all the interventions to the chart. We are going to end up plotting plotting interventions for variables like I, S, and R all on the same chart. It will create a issue such that chart for variable I also have intervention for other variables, for example S or R along with all interventions for parameters as well. I'm not sure if that's what we want to do. If we want still draw all interventions for parameters to the chart, we should at least filter out the intervention for non target state variables (e.g. S, R etc) I guess and only plot the interventions for the parameters (e.g. beta, alpha etc).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is something @pascaleproulx needs to figure out with the design team. So until they do this, that PR can be a step forward, and we should merge it as a first version.

Copy link
Contributor

@jryu01 jryu01 Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I think we should include the interventions for the the same variable if it's for state variable, and all for parameters and filter out the intervention for other state variables. For example, chart for I should include the interventions for I only and all interventions for the parameters (e.g beta) on top of it. @mloppie you can probably use modelPartTypesMap in line 590 to check the type to see if a variable is state variable or a parameter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I still think that without proper design how we want to show parameter intervention on the state chart, just plotting all parameter interventions markers as vertical lines on the state chart won't be that useful. Parameter intervention were intentionally left out I believe. And this is going to be reworked depending on how we want to do it properly. I feel it's wise to pause until we get better idea how we should draw them on the chart from Pascale.

return charts;
});
Expand Down
7 changes: 5 additions & 2 deletions ...nt/hmi-client/src/components/workflow/ops/calibrate-ciemss/tera-calibrate-node-ciemss.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,11 @@ const preparedCharts = computed(() => {
}
);
applyForecastChartAnnotations(chart, annotations);
chart.layer.push(...createInterventionChartMarkers(groupedInterventionOutputs.value[variable]));

Object.entries(groupedInterventionOutputs.value).forEach((item) => {
item[1].forEach((intervention) => {
chart.layer.push(...createInterventionChartMarkers([intervention], false, -80));
YohannParis marked this conversation as resolved.
Show resolved Hide resolved
});
});
return chart;
});
});
Expand Down
5 changes: 3 additions & 2 deletions packages/client/hmi-client/src/services/charts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,8 @@ export function createSuccessCriteriaChart(

export function createInterventionChartMarkers(
data: ReturnType<typeof flattenInterventionData>,
hideLabels = false
hideLabels = false,
labelXOffset = 5
): any[] {
const markerSpec = {
data: { values: data },
Expand All @@ -742,7 +743,7 @@ export function createInterventionChartMarkers(
type: 'text',
align: 'left',
angle: 90,
dx: 5,
dx: labelXOffset,
dy: -10
YohannParis marked this conversation as resolved.
Show resolved Hide resolved
},
encoding: {
Expand Down