-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Add quiver trace type for vector field visualization #7584
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
base: master
Are you sure you want to change the base?
Conversation
dfd3c81 to
9463b3f
Compare
|
Hello, I am new to this project and attempting to contribute my first PR. Unfortunately, I am struggling to get the build to pass. Specifically, the I tried running But still encountering the issue. Any insight would be greatly appreciated. Thank you! Edit: tagging a few recent commiters for visibility, thanks so much! (cc @camdecoster @alexshoe @emilykl) |
|
Hi @degzhaus, thank you for working on this contribution! As it stands currently, the devtools dashboard needs to be running in order for You can also download |
b07de7b to
a32678d
Compare
Thank you so much, @emilykl! That was very helpful and unblocked me.
Edit: @emilykl i figured it out! Pardon my flailing at last night. So exciting! I have a couple of tests to add and this will be ready for review. Thanks again for your insights and help! |
64cf31d to
02c37f5
Compare
|
@degzhaus Great news, glad you're unblocked! Thank you for your work on this. Quiver is a plot type we've gotten a lot of requests for. A few high-level comments on the API:
|
Amazing, thank you so much for taking a look and providing great guidance, Emily! Looking forward to spinning a cycle on this feedback. |
|
@degzhaus One more comment — it would be great to include colorscale attributes in |
|
Perhaps for future readers' clarity of understanding, @degzhaus , you could edit the description at the top of the PR to say:
in the two places it appears, so that the as-merged plot type appears there. |
b93b445 to
0252e03
Compare
Great catch, thank you so much, @gpdf ! Updated per your recommendation, and did a little review/revision based on changes made during the review process as well. |
Hey there, @emilykl, hope you're having a wonderful week! I rebased, cleaned up some code, and added some tests here: Thanks again for everything! |
|
I really appreciate all the effort in polishing, and very much look forward to using this. |
|
@emilykl Where are we on the trajectory toward getting this merged? I'm excited about being able to start using it. |
|
@emilykl Is there anything we can do on our end to support this? |
0252e03 to
51a2afc
Compare
|
Hi @degzhaus ! I sincerely apologize for the delay in the next round of review, and a big thank you for your patience. I'll be going through this PR again this afternoon. Thanks again for your work so far and for persevering with this one! |
51a2afc to
2b2d1c9
Compare
All good, @emilykl ! Thank you so much for the message. I just rebased and ensured tests are passing, so looking forward to the next steps of the review. Thanks again to you both, @emilykl @gpdf ! John |
|
Hi @degzhaus ! Sorry for the delay. Thanks for your continued work. I remember noticing a bug on a previous iteration where the arrow styling was lost when the graph was panned; that seems to be fixed now. 🎉 However I do see some remaining items that should be addressed: Bugs
API refinements (for consistency with other traces)Open to discussion on any of these if you have an argument for keeping as-is, or another suggestion.
I'll leave some comments on the code as well if anything jumps out at me, but those are the big picture items! Feel free to reach out with questions on any of these, and I'll get back to you more quickly next time around. 😉 Thanks again. |
| // Coerce x and y data arrays (this ensures proper data structure for category ordering) | ||
| var x = coerce('x'); | ||
| var y = coerce('y'); | ||
| var u = coerce('u'); | ||
| var v = coerce('v'); |
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.
@degzhaus Can you look into whether we can/should use the handleXYDefaults() function here, as done for the scatter trace?
Reusing the same helper function would be ideal if it works without too much massaging.
| coerce('vhoverformat'); | ||
|
|
||
| // Colorscale defaults (adds colorscale, showscale, colorbar, etc.) | ||
| traceOut._hasColorscale = true; |
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.
Should probably call the hasColorscale() function here (although I'm curious how the scatter trace manages without the _hasColorscale field, worth looking into)
| @@ -0,0 +1,37 @@ | |||
| 'use strict'; | |||
|
|
|||
| module.exports = function selectPoints(searchInfo, selectionTester) { | |||
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.
Could we plausibly reuse the src/traces/scatter/select.js selectPoints() function instead of duplicating a lot of that code here? The implementations look almost identical, although not quite.
Overview
This PR adds a new
quivertrace type to Plotly.js for visualizing 2D vector fields using arrows.Features
x,ycoordinates with direction/magnitude fromu,vcomponentsscaled(normalized),absolute, orrawvector lengths viasizemode/sizereftail,tip, orcenter/cm/middlecarray)line.*attributesarrowsizehovertemplate, point selectionx,y,u,v, andcarraysAPI
Screenshots
Examples taken from plotly.com/python/quiver-plots
Gist with example code
Basic Quiver Plot
With Colorscale
Testing
Files Changed
New files (
src/traces/quiver/)index.js- Trace module definitionattributes.js- Attribute schemadefaults.js- Default value handlingcalc.js- Data calculationplot.js- SVG renderingstyle.js- Stylinghover.js- Hover behaviorselect_points.js- Selection supportevent_data.js- Event data formattingformat_labels.js- Label formattingModified
lib/index.js&lib/index-strict.js- Build integrationTests
test/jasmine/tests/quiver_test.js- Unit teststest/image/mocks/quiver_*.json- 9 mock filestest/image/baselines/quiver_*.png- Baseline images