-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Some error bar clean ups #91
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
Conversation
- compute_error is the reusable piece of logic that gl3d and gl2d error bars will use.
* - error[0] : error magnitude in the negative direction | ||
* - error[1] : " " " " positive " | ||
*/ | ||
module.exports = function computeError(dataPt, index, opts) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all about the 🐎
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.
}; | ||
} | ||
else { | ||
return function computeError(dataPt, index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bpostlethwaite @mdtusz better?
} | ||
if(type === 'sqrt') { | ||
return function(dataPt) { | ||
return Math.sqrt(Math.abs(dataPt)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
@alexcjohnson @mdtusz @cldougl @bpostlethwaite
This PR:
type: 'sqrt'
type: 'sqrt'
(about time)default
,calc
and reusable compute-error-magnitude routine into separate files