Skip to content

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Dec 8, 2015

@alexcjohnson @mdtusz @cldougl @bpostlethwaite

This PR:

  • fixes default logic for error bar type: 'sqrt'
  • adds test for error bar type: 'sqrt' (about time)
  • reuse svg error bar logic in 3d convert step
  • split error bar default, calc and reusable compute-error-magnitude routine into separate files
  • remove an used function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reusable error bar data/constant/sqrt/percent logic

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good to me. Don't need to do it now, but it occurs to me we could boost performance by just evaluating opts once and returning a more minimal function to evaluate in the loop:

function computeError(opts) {
    if(type === 'data') {
        if(opts.symmetric || opts.arrayminus === undefined) {
            var array = opts.array
            return function(dataPt, index) {
                var val = +(array[index]);
                return [val, val];
            }
        }
        // etc...
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all about the 🐎

@alexcjohnson
Copy link
Collaborator

Thanks @etpinard , 💃 for my part.

- browserify doesn't like them
- so that the opts object is destructure only once once per
  trace instead of on every data pt.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson is this what you had in mind?

Copy link
Collaborator

Choose a reason for hiding this comment

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

😍

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like an ideal place to use a switch statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've been reluctant to use swtich statements in plotly.js so far. Maybe this is the time to use them more.

@bpostlethwaite switch statements are kinda messy in js, are they considered a good pattern?

etpinard added a commit that referenced this pull request Dec 14, 2015
@etpinard etpinard merged commit edcfe7e into master Dec 14, 2015
@etpinard etpinard deleted the better-errorbars branch December 14, 2015 17:08
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.

4 participants