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.

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

Jump to bottom

Fix parcoords dimensions values & line coloring error with integer TypedArray #3598

Merged
merged 5 commits into from
Mar 4, 2019
Merged

Fix parcoords dimensions values & line coloring error with integer TypedArray #3598

merged 5 commits into from
Mar 4, 2019

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Mar 3, 2019 •
edited
Loading

Fixes two issues reported & discovered in #3595 resulted from integer rounding errors.

  • case of color.line typed arrays e.g. int32
  • case of dimensions.values typed arrays e.g. int32

@plotly_js
Codepen Before.
Codepen After.

@antoinerg
Copy link
Contributor

Thanks for the fix @archmoj! It looks good to me 👍

I am wondering if we should add a parcoords mock with line.color as a typedArray to 🔒 this down.

@archmoj
Copy link
Contributor Author

archmoj commented Mar 3, 2019

@antoinerg thanks for the review & comment. I am not sure if we could make a JSON mock with typed arrays. So I may add a jasmine test.

@archmoj archmoj changed the title Fix parcoords line coloring error with integer TypedArray Fix parcoords dimensions values & line coloring error with integer TypedArray Mar 3, 2019
@archmoj
Copy link
Contributor Author

archmoj commented Mar 3, 2019

@antoinerg Please note that e8cc760 is also added concerning my comment here.

@archmoj archmoj mentioned this pull request Mar 3, 2019
Closed
@antoinerg
Copy link
Contributor

I am not sure if we could make a JSON mock with typed arrays.

You're probably right 😄 Thanks for the jasmine tests!

I confirm this PR fix both issues in #3595 but I wonder why coerce isn't already doing the right thing and whether copying the TypedArray into an Array is avoidable. Therefore, I will leave it up to @etpinard to give you a dancer!

Nice work @archmoj

etpinard
etpinard reviewed Mar 4, 2019
View reviewed changes
src/traces/parcoords/defaults.js Outdated
@@ -49,6 +49,11 @@ function dimensionDefaults(dimensionIn, dimensionOut) {
}

var values = coerce('values');
if(!Array.isArray(values) && Lib.isArrayOrTypedArray(values)) {
// should convert typed arrays e.g. integers to real numbers
values = dimensionOut.values = Array.prototype.slice.call(values);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nop. Let's not do this during defaults to remain consistent with other typed-array-to-array blocks.

Move this to parcoords/calc.js.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we can keep typed arrays as typed array and handle them correct downstream, then we shouldn't convert them.

That makes perfect sense!

etpinard
etpinard reviewed Mar 4, 2019
View reviewed changes
src/traces/parcoords/defaults.js Outdated
var lineColor = coerce('line.color', defaultColor);
if(!Array.isArray(lineColor) && Lib.isArrayOrTypedArray(lineColor)) {
// should convert typed arrays e.g. integers to real numbers
lineColor = traceOut.line.color = Array.prototype.slice.call(lineColor);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here.

@etpinard
Copy link
Contributor

etpinard commented Mar 4, 2019 •
edited
Loading

but I wonder why coerce isn't already doing the right thing and whether copying the TypedArray into an Array is avoidable

because we don't want to do that in general. If we can keep typed arrays as typed array and handle them correctly downstream, then we shouldn't convert them. See for example ax.makeCalcdata which is called by most trace modules:

plotly.js/src/plots/cartesian/set_convert.js

Lines 516 to 537 in ecfa45a

if(axLetter in trace) {
arrayIn = trace[axLetter];
len = trace._length || Lib.minRowLength(arrayIn);
if(Lib.isTypedArray(arrayIn) && (axType === 'linear' || axType === 'log')) {
if(len === arrayIn.length) {
return arrayIn;
} else if(arrayIn.subarray) {
return arrayIn.subarray(0, len);
}
}
if(axType === 'multicategory') {
return setMultiCategoryIndex(arrayIn, len);
}
arrayOut = new Array(len);
for(i = 0; i < len; i++) {
arrayOut[i] = ax.d2c(arrayIn[i], 0, cal);
}
}
else {

@etpinard
Copy link
Contributor

etpinard commented Mar 4, 2019

I am not sure if we could make a JSON mock with typed arrays.

See #913

etpinard
etpinard reviewed Mar 4, 2019
View reviewed changes
src/traces/parcoords/calc.js Outdated
@@ -39,3 +45,11 @@ function constHalf(len) {
}
return out;
}

function isTypedArray(a) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this. Lib.isTypedArray is a thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard Thanks for the note.
Done in e577e49.

@etpinard
Copy link
Contributor

etpinard commented Mar 4, 2019

Nicely done 💃

@archmoj archmoj merged commit dab105c into master Mar 4, 2019
@archmoj archmoj deleted the parcoords-line-color-integer branch March 4, 2019 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoinerg antoinerg antoinerg left review comments

@etpinard etpinard etpinard left review comments

Assignees
No one assigned
Labels
status: reviewable type: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@archmoj @antoinerg @etpinard