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.

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

Add: error bands support to graphs (all except pie) #1404

Closed
wants to merge 34 commits into from

Conversation

luke14free
Copy link
Contributor

@luke14free luke14free commented Nov 17, 2016

f122d760-ac24-11e6-9821-a5c5935b3be5

Closes #795, #1188.

@luke14free luke14free mentioned this pull request Nov 17, 2016
@arikfr arikfr changed the title Adding errors to graphs (all except pie) Add: error bands support to graphs (all except pie) Nov 17, 2016
@@ -260,13 +266,13 @@ function QueryResultService($resource, $timeout, $q) {

if (seriesName === undefined) {
each(yValues, (yValue, ySeriesName) => {
addPointToSeries({ x: xValue, y: yValue }, series, ySeriesName);
errors.push(eValue);
addPointToSeries({ x: xValue, y: yValue, error_y: eValue }, series, ySeriesName);
Copy link
Member

Choose a reason for hiding this comment

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

Should be errorY or yError.

@@ -218,12 +218,14 @@ function QueryResultService($resource, $timeout, $q) {

getChartData(mapping) {
const series = {};
const errors = [];
Copy link
Member

Choose a reason for hiding this comment

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

Seems unused?

@@ -248,6 +249,10 @@ function QueryResultService($resource, $timeout, $q) {
yValues[name] = value;
point[type] = value;
}
if (type === 'error_y') {
Copy link
Member

Choose a reason for hiding this comment

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

for consistency let's change this too to yError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mh, I am a bit confused by the implementation of the whole method. But I believe it needs to be error_y because it's already the data structure that is feed to plotly (and the parameter that plotly want as input is called error_y). I will double check anyways

data.forEach((row) => { yValues[row.x] = row.y; });
data.forEach((row) => {
yValues[row.x] = row.y;
eValues[row.x] = row.yError;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we set eValues only if they are actually exist?

@arikfr
Copy link
Member

arikfr commented Nov 24, 2016

Seems like it picked up some of my commits... anyway you need to rebase :|

@luke14free
Copy link
Contributor Author

luke14free commented Nov 25, 2016

some stuff was seriously wrong with this PR, I was totally unable to rebase, I am closing this one and opening #1430

@luke14free luke14free closed this Nov 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants