[FIX] charts: non-invertible matrix returns NaN as predicted data #5202
Conversation
|
|
| predictLinearValues([values], [normalizedLabels], [normalizedNewLabels], true) | ||
| )[0]; | ||
| } catch (e) { | ||
| return []; |
There was a problem hiding this comment.
why do we return an empty array here, but an array of NaN in th'e other cases ? Cna't we return an empty array everytime in a global try/catch on the function ?
There was a problem hiding this comment.
The empty array is a mistake, it should returns NaN every time. In fact, the idea of returning NaN was to see the series in the legend, while an empty array gives no entry in the legend.
There was a problem hiding this comment.
This means that we end up displaying a legend for an invisible dataset since it only contains NAN values. I don't really see the point - the user will just wonder what went wrong. Hiding the legend doesn't do the trick either. I think the error should be somehow visible in the sidepanel if something went wrong?
There was a problem hiding this comment.
I totally agree with that point of showing the error, but should it be done in v18 or in master then ? As it will change some part of the structure of the side panel to be able to display an error message (not a big change, but one change "quand-même"
| expect(runtime.chartJsConfig.data.datasets[1].data.every((x) => isNaN(Number(x)))).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
Shoudln't we avoid giving an useless dataset filled with NaN to chartJS ? At some points we will change something and it chartJs will end up putting it in a legend or a tooltip or something.
anhe-odoo
force-pushed
the
18.0-chart-handle_trendline_error-anhe
branch
from
63adf97
to
0012c66
Compare
Task Description When trying to make a trending line, almost all model use matrix computation to get the predicted data, with some matrix multiplications and inversions. When the matrixes aren't invertible the user get a traceback, which is not fine. This PR aims to fix these issues by returning an array of NaN when there is an error in the computation of the predicted dataset. Related Task Task: 4328743
anhe-odoo
force-pushed
the
18.0-chart-handle_trendline_error-anhe
branch
from
0012c66
to
8ff8278
Compare
| try { | ||
| switch (config.type) { | ||
| case "polynomial": { | ||
| const order = config.order ?? 2; |
There was a problem hiding this comment.
Shouldn't this crash/return early if the order is not provided? We know that the config was badly done at that point. Do youthink this should be checked by the allowDispatch?
Task Description
When trying to make a trending line, almost all model use matrix computation to get the predicted data, with some matrix multiplications and inversions. When the matrix aren't inversibles, the user get a traceback, which is not fine.
This PR aims to fix these issues by returning an array of NaN when there is an error in the computation of the predicted dataset.
Related Task
review checklist