Skip to content

use supplied bandwidth if provided #2552

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 22, 2018
Merged

use supplied bandwidth if provided #2552

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/traces/violin/calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,12 @@ module.exports = function calc(gd, trace) {

// sample standard deviation
var ssd = Lib.stdev(vals, len - 1, cdi.mean);
var bandwidthDflt = ruleOfThumbBandwidth(vals, ssd, cdi.q3 - cdi.q1);
var bandwidth = cdi.bandwidth = trace.bandwidth || bandwidthDflt;
var bandwidth = cdi.bandwidth = trace.bandwidth || ruleOfThumbBandwidth(vals, ssd, cdi.q3 - cdi.q1);
Copy link
Contributor

Choose a reason for hiding this comment

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

So for your data I suspect bandwidthDflt equals 0. Is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bandwidthDflt = 3.958842142868651e-8

var span = cdi.span = calcSpan(trace, cdi, valAxis, bandwidth);

// step that well covers the bandwidth and is multiple of span distance
var dist = span[1] - span[0];
var n = Math.ceil(dist / (Math.min(bandwidthDflt, bandwidth) / 3));
var n = Math.ceil(dist / (bandwidth / 3));
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for choosing just bandwidth here instead of bandwidthDflt ? Math.min(bandwidthDflt, bandwidth) : bandwidth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I take the min, then n=103656617

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok. In this case, I'm thinking capping n would be best. Probably n = Math.max(n, 1e5) should suffice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably n = Math.max(n, 1e5) should suffice

You mean Math.min, but agreed. As is (with or without this PR) you could crash the browser just by setting a small bandwidth. @etpinard was there a reason to use Math.min(bandwidthDflt, bandwidth) before? Seems like 3 points per bandwidth will get the curve shape correct even if you set a large explicit bandwidth, won't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

was there a reason to use Math.min(bandwidthDflt, bandwidth) before?

Good question. It comes from the very first violin commit. I must have taken that (possibly erroneously) from the seaborn implementation.

var step = dist / n;

if(!isFinite(step) || !isFinite(n)) {
Expand Down