Skip to content

scattergl errorbars fixes - #2620

Merged
etpinard merged 6 commits into
masterfrom
scattergl-errors
May 22, 2018
Merged

scattergl errorbars fixes#2620
etpinard merged 6 commits into
masterfrom
scattergl-errors

Conversation

@etpinard

Copy link
Copy Markdown
Contributor

Fixes#2600 and the last item off #2450 (comment) (x errorbars aren't supported?).

Does not attempt to fix the on-relayout issue of #2450 (comment) nor the incorrect errorbar layering order described in #2450, @dy will take care of them once he finishes his regl-component module.

cc @alexcjohnson

- for example, when 'x' array is given (i.e. the x0/dx case)
or when numeric string are inputted.
Comment threadsrc/traces/scattergl/convert.js Outdated
var calcFromTrace = Registry.getComponentMethod('errorbars', 'calcFromTrace');
var vals = calcFromTrace(trace, gd._fullLayout);
var count = positions.length / 2;
var _trace = Lib.extendFlat({}, trace, {x: x, y: y});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This seems weird to me - putting arrays of calcdata values into a fake trace. I don't know if category or date data would make any sense in this context but if we were to support them this would break.

This is the only caller of calcFromTrace, right? Can we just pass in the x and y calc'ed arrays separately from the trace?

@etpinardetpinardMay 22, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good call. Using calcFromTrace made things very dry 🌴 but added a lot of overhead and many (many) object creations. So yeah good riddance 🔪 -> 82290a2

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah nice - that's more of a refactor than I was expecting but you're right, much better. 🌴 before, but as well as the 🐎 overhead, the scaffolding meant it didn't even really result in shorter code.

Comment threadsrc/traces/scattergl/convert.js Outdated
var opts = trace['error_' + axLetter];

if(opts && opts.visible && (ax.type === 'linear' || ax.type === 'log')) {
var computeError = makeComputeError(trace['error_' + axLetter]);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

makeComputeError(opts)?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

good eye!

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@alexcjohnson

Copy link
Copy Markdown
Collaborator

Very nice! 💃 after the minor comment above.

@etpinard
etpinard merged commit e004a09 into masterMay 22, 2018
@etpinard
etpinard deleted the scattergl-errors branch May 22, 2018 17:23
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugsomething broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

scattergl calculates wrong error bar value if y value is a string

2 participants

@etpinard@alexcjohnson