Skip to content

Handle streamtube coordinates in string format and handle different data orders in isosurface and volume - #4431

Merged
archmoj merged 11 commits into
masterfrom
fix4427-streamtube-string-coords
Dec 30, 2019
Merged

Handle streamtube coordinates in string format and handle different data orders in isosurface and volume#4431
archmoj merged 11 commits into
masterfrom
fix4427-streamtube-string-coords

Conversation

@archmoj

@archmojarchmoj commented Dec 18, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes#4427:
Whereas raw x, y and z values were used in streamtube calc step, in order to find the order of data, it was necessary to ensure casting to numbers before comparing two values.
Before
After

And fixes#4440:
Similar to #4271, this PR fixed the data cube order issue so that the value as well as x, y and z arrays could be filled in different orders.
Before
After

@plotly/plotly_js

@archmojarchmoj added bug something broken status: reviewable regression this used to work labels Dec 18, 2019
etpinard
etpinard reviewed Dec 18, 2019
View reviewed changes
Comment threadsrc/traces/streamtube/calc.js
etpinard
etpinard reviewed Dec 23, 2019
View reviewed changes
Comment threadsrc/traces/streamtube/calc.js Outdated
if(len === undefined) len = arr.length;

// no need for casting typed arrays to numbers
if(Lib.isTypedArray(arr)) return arr.slice(0, len);

@etpinardetpinardDec 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's better to use arr.subarray() for typed arrays, as .slice isn't supported in older browsers.

@archmojarchmojDec 23, 2019

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.
Wondering if we need to slice the array at all?

@archmojarchmojDec 24, 2019

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.

slice (or subarry) is needed for distictVals to work correctly.
But it may be better not to slice if the length is correct.

@etpinardetpinardDec 27, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But it may be better not to slice if the length is correct.

that could be a nice optimisation for typed arrays, but for regular arrays we'll need to slice in general to cast numerical-strings items w/o mutating.

@archmoj
create a reusable process grid function
d015b57
- computed trace._len and process grid at the start of calc
- apply subarray instead of slice for typed arrays
- improve streamtube jasmine test
@archmoj
Apply streamtube processGrid in isosurface and volume
5a95dab
- add image tests for various grid fill orders
- add jasmine tests for various grid fill orders and data types
@archmojarchmoj changed the title Handle streamtube coordinates in string formatHandle streamtube coordinates in string format and handle different data orders in isosurface and volumeDec 24, 2019
@archmojarchmoj added this to the v1.52.0 milestone Dec 25, 2019
etpinard
etpinard reviewed Dec 27, 2019
View reviewed changes
Comment threadsrc/traces/isosurface/convert.js
etpinard
etpinard reviewed Dec 27, 2019
View reviewed changes
Comment threadsrc/traces/streamtube/calc.js
etpinard
etpinard reviewed Dec 27, 2019
View reviewed changes
Comment threadsrc/traces/streamtube/calc.js
etpinard
etpinard reviewed Dec 27, 2019
View reviewed changes
Comment threadsrc/traces/streamtube/calc.js Outdated
@archmoj
add an image test for volume with different fill orders
6d06229
@archmoj
cast x, y, z, u, v, w, values to numbers at the start of calc and sto…
df9b4c4
…re them at trace._x etc.
- rename previous trace._x, _y, _z & _intensity as well as trace._i, _j & _k to trace._meshX, etc. to avoid name conflicts
@archmoj
remove extra cast operations
41504aa
etpinard
etpinard reviewed Dec 30, 2019
View reviewed changes
Comment threadsrc/traces/isosurface/calc.js Outdated
etpinard
etpinard reviewed Dec 30, 2019
View reviewed changes
Comment threadsrc/traces/streamtube/calc.js Outdated
@etpinard

etpinard commented Dec 30, 2019

Copy link
Copy Markdown
Contributor

Nice job @archmoj - this PR is looking very nice now, one commit away from merging.

@etpinard

etpinard commented Dec 30, 2019

Copy link
Copy Markdown
Contributor

Beautiful fix!! Thanks very much for bringing some sanity into streamtube-land

💃 💃 💃

@archmoj
archmoj merged commit ed26d23 into masterDec 30, 2019
@archmoj
archmoj deleted the fix4427-streamtube-string-coords branch December 30, 2019 16:11
@jdamibajdamiba mentioned this pull request Jan 9, 2020
Closed
@snyk-botsnyk-bot mentioned this pull request Feb 4, 2020
Merged
@snyk-botsnyk-bot mentioned this pull request Feb 22, 2020
Open
@snyk-botsnyk-bot mentioned this pull request Mar 18, 2020
Open
@snyk-botsnyk-bot mentioned this pull request Apr 17, 2020
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@etpinardetpinardetpinard left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bugsomething brokenregressionthis used to work

Projects

None yet

Milestone

v1.52.0

Development

Successfully merging this pull request may close these issues.

Isosurface and volume expecting certain input data order Streamtube problem input string numbers

2 participants

@archmoj@etpinard