Add quiver trace type for vector field visualization#7710
Conversation
1262c1e to
f327f5d
Compare
|
Thanks again for the great comments, @emilykl ! I will attempt to reference commits inline from this comment here: -- Bug Fixes -- "The axis ranges don't automatically adjust to include the full length of the arrows" fixed here: "Colorscale seems broken in the latest version" fixed here: "Legends are missing icons" fixed here: " "The "Display of colorbars does not seem to work" fixed here: -- API Refinements -- " "The line options should be nested under marker.line", |
|
Here are the followups for the remaining 3 comments from: #7584, @emilykl : "Could we plausibly reuse the src/traces/scatter/select.js selectPoints() function" fixed here: "Should probably call the hasColorscale() function here" fixed here: "Can you look into whether we can/should use the handleXYDefaults() function here" fixed here: |
|
Alrighty, @emilykl , thanks again for your great feedback and patience as I addressed it! I have pushed all my fixes and tested locally, I think we're close 🤞 |
|
Great news @degzhaus ! Thank you for the continued work and the clear summary. I will try to take another look within the next week. Feel free to ping me next Tuesday if I haven't gotten back to you by then. |
acfd7f0 to
1a15aa3
Compare
|
hey there, @emilykl, hope you are having a great start to the week! i just rebased and pushed again 🥳 |
|
@degzhaus would you be able to merge/rebase one more time? Sorry for the extra work. We just changed how our test images are generated and that led to the baseline conflicts with this branch. |
|
Hi @degzhaus ! I've run down the list of issues from my previous review, and most seem to be fixed 🥳 However I am still seeing these two issues:
The baseline images
The baseline images in I'm still doing a bit more testing so I will leave another comment if I find anything else. @camdecoster is taking a look as well, so keep an eye out for his comments. Finally when you have a chance please merge/rebase against the master branch! We recently overhauled the CI pipeline and you will need to merge/rebase in order to run the new pipeline. |
| }, | ||
| anchor: { | ||
| valType: 'enumerated', | ||
| values: ['tip', 'tail', 'cm', 'center', 'middle'], |
There was a problem hiding this comment.
Could you please remove 'cm' and 'center' as values? One term for the middle should be fine.
There was a problem hiding this comment.
Much simpler, thanks, removed redundant anchor value options:
| }; | ||
|
|
||
| // Extend with base attributes (includes hoverinfo, etc.) | ||
| extendFlat(attrs, baseAttrs); |
There was a problem hiding this comment.
Did you mean to pasate baseAttrs on top of attrs? I think it would be safer to handle it this way. That's what is typically done when building attributes.
| extendFlat(attrs, baseAttrs); | |
| attrs = extendFlat({}, baseAttrs, attrs); |
There was a problem hiding this comment.
Good call, thank you, changed here:
| var trace = cd[0].trace; | ||
| var marker = trace.marker || {}; | ||
| var markerLine = marker.line || {}; | ||
| var lineColor = Lib.isArrayOrTypedArray(marker.color) ? undefined : marker.color; |
There was a problem hiding this comment.
If marker.color is an array, lineColor will be undefined. This would result in the unselected color being undefined as well. Could you update the code to handle this situation and apply a dim version (via opacity) of the selected color?
There was a problem hiding this comment.
Thanks for pointing this out. I was not able to figure out exactly how to do that with opacity, so I styled the path with stroke-opacity here:
Happy to adjust if you have something else in mind!
|
|
||
| // Colorscale cmin/cmax computation: prefer provided marker.color, else magnitude | ||
| if(trace._hasColorscale) { | ||
| var vals = hasMarkerColorArray ? [cMin, cMax] : [normMin, normMax]; |
There was a problem hiding this comment.
Could you add a guard (here or elsewhere) that handles if normMin and normMax are infinite? This is an edge case that would require all invalid data, but we should guard against it.
There was a problem hiding this comment.
Good callout, thank you, Cam, added guard here:
| var Lib = require('../../../src/lib'); | ||
| var d3Select = require('../../strict-d3').select; |
There was a problem hiding this comment.
Could you delete these unused imports?
There was a problem hiding this comment.
Absolutely, removed here:
|
@degzhaus are you still interested in finishing up this PR? |
i am, and pardon my delay, Cam! i have begun working through the recent comments, hope to be ready for another round of review this week 🤞 |
|
Great to hear! Ping us when you're ready for another review. |
|
Hello @degzhaus! We're working toward a new major release and we're interested in including this new trace as part of that. If we want that to happen, this PR will need to be merged in the next week or so. Do you think you'll have time to finish it up by then? We're happy to help if you need some assistance. |
Amazing, that is so exciting! Thanks so much for y'all's direction, @emilykl and @camdecoster, I learned a lot working with each of you. I responded inline with what I think are the remaining outstanding points. Very excited for final steps. Thanks so much for reaching out to be sure this is included in the release, that means a lot. Feel free to ping with any remaining tasks, I will keep my eyes peeled for updates. All the best. |
|
I believe that you've addressed all of my comments. I'll let @emilykl take a look and give the final review. I'm going to edit this PR to target our 4.0 branch instead of master. Then I'll merge the 4.0 changes into your branch and cross my fingers that it just works. I'll push those changes and then it should good to go for Emily to look at. |
|
Hey! It worked! It looks like you just need to update the baseline images, address the failing test, and add a draftlog entry (plus whatever Emily comments on). Almost there! |
Great news, and thank you so much for hopping on this today and taking care of those requisite commits, @camdecoster ! I just pushed 3 more commits as well, hopefully CI will be as happy as me 🥳 Is there something I can do to initiate the pipeline or will that happen automatically? Thanks again for everything, Cam and Emily 🙌 |
|
I approved the workflows. All green. |
|
@degzhaus Exciting news! I'm starting to review, I'll get back to you ASAP with any comments. |
There was a problem hiding this comment.
@degzhaus I think it would be OK to remove this image test (and the associated PNG) since as far as I can tell it doesn't test any properties which aren't covered by the other mocks.
There was a problem hiding this comment.
@degzhaus Instead of testing this in a separate mock, could you edit one of the other mocks to include zero-length vectors and remove this one?
(Each image test adds a bit of CI time, so we want to strike a balance between having comprehensive tests while also keeping the number of image tests down.)
| ['layout-colorway', require('../../image/mocks/layout-colorway.json')], | ||
| ['multicategory', require('../../image/mocks/multicategory.json')], | ||
| ['polar_categories', require('../../image/mocks/polar_categories.json')], | ||
| ['quiver_simple', require('../../image/mocks/quiver_simple.json')], |
There was a problem hiding this comment.
I don't think this is needed
| ['quiver_simple', require('../../image/mocks/quiver_simple.json')], |
| // Depending on prior tests, the suppression warning may already be reached once. | ||
| expect(Lib.warn.calls.count()).toBeGreaterThanOrEqual(10); | ||
| expect(Lib.warn.calls.count()).toBeLessThanOrEqual(11); |
There was a problem hiding this comment.
@camdecoster what do you think about this patch for this flaky test? It loosens the test, but we already re-run until it passes anyway, so maybe it's fine to just loosen the test a bit to maintain our sanity.
There was a problem hiding this comment.
I'm okay with it. I ran into that test with the hovertemplate work I did. It's not a great test and should be updated when we switch to a new testing library.
| hoverdistance: { | ||
| valType: 'number', | ||
| min: -1, | ||
| dflt: 20, | ||
| editType: 'calc', | ||
| description: 'Maximum distance (in pixels) to look for nearby arrows on hover.' | ||
| }, |
There was a problem hiding this comment.
No other trace type has a hoverdistance attribute (it's a layout property) so I'd suggest to remove it unless there's a good reason to define it at the trace level here.
| hoverdistance: { | |
| valType: 'number', | |
| min: -1, | |
| dflt: 20, | |
| editType: 'calc', | |
| description: 'Maximum distance (in pixels) to look for nearby arrows on hover.' | |
| }, |
| text: { | ||
| valType: 'data_array', | ||
| editType: 'calc', | ||
| anim: true, | ||
| description: 'Sets text elements associated with each (x,y) pair.' | ||
| }, | ||
| textposition: { | ||
| valType: 'enumerated', | ||
| values: [ | ||
| 'top left', 'top center', 'top right', | ||
| 'middle left', 'middle center', 'middle right', | ||
| 'bottom left', 'bottom center', 'bottom right' | ||
| ], | ||
| dflt: 'middle center', | ||
| editType: 'calc', | ||
| description: 'Sets the positions of the `text` elements with respects to the (x,y) coordinates.' | ||
| }, | ||
| // Text font | ||
| textfont: fontAttrs({ | ||
| editType: 'calc', | ||
| colorEditType: 'style', | ||
| arrayOk: true, | ||
| description: 'Sets the text font.' | ||
| }), |
There was a problem hiding this comment.
Preference for reusing the scatter attributes here if possible. Can use extendFlat if a few attribute properties need to be changed.
Of course if the whole attribute definition really does need to be different, it's OK to redefine here.
| text: { | |
| valType: 'data_array', | |
| editType: 'calc', | |
| anim: true, | |
| description: 'Sets text elements associated with each (x,y) pair.' | |
| }, | |
| textposition: { | |
| valType: 'enumerated', | |
| values: [ | |
| 'top left', 'top center', 'top right', | |
| 'middle left', 'middle center', 'middle right', | |
| 'bottom left', 'bottom center', 'bottom right' | |
| ], | |
| dflt: 'middle center', | |
| editType: 'calc', | |
| description: 'Sets the positions of the `text` elements with respects to the (x,y) coordinates.' | |
| }, | |
| // Text font | |
| textfont: fontAttrs({ | |
| editType: 'calc', | |
| colorEditType: 'style', | |
| arrayOk: true, | |
| description: 'Sets the text font.' | |
| }), | |
| text: scatterAttrs.text, | |
| textposition: scatterAttrs.textposition, | |
| textfont: scatterAttrs.textfont, |
| attrs.hoverinfo = extendFlat({}, baseAttrs.hoverinfo, { | ||
| flags: ['x', 'y', 'u', 'v', 'text', 'name'], | ||
| dflt: 'all' | ||
| }); | ||
|
|
||
| // Add hovertemplate | ||
| attrs.hovertemplate = extendFlat({}, hovertemplateAttrs({}, { | ||
| keys: ['x', 'y', 'u', 'v', 'text', 'name'] | ||
| })); |
There was a problem hiding this comment.
@degzhaus Is there a reason why these are added afterwards, rather than defined in the main body of attrs?
| var y; | ||
|
|
||
| var hasOnlyLines = (!subtypes.hasMarkers(trace) && !subtypes.hasText(trace)); | ||
| var hasOnlyLines = trace.mode && !subtypes.hasMarkers(trace) && !subtypes.hasText(trace); |
There was a problem hiding this comment.
IMO it would be clearer to modify the subtypes.hasMarkers(trace) logic such that it returns true for quiver traces, rather than adding a trace.mode escape hatch here.
|
FYI @degzhaus I've left some comments but haven't quite finished my review yet, I'll continue on Monday! |
Overview
This PR adds a new
quivertrace type to Plotly.js for visualizing 2D vector fields using arrows.Continuation of #7584
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