Constraint-type contours for regular contour (not on a carpet)#2270
Constraint-type contours for regular contour (not on a carpet)#2270alexcjohnson merged 10 commits intomasterfrom
Conversation
it never worked in contourcarpet, was left in accidentally I guess
etpinard
left a comment
There was a problem hiding this comment.
Solid PR 💪
I love the way contour and contourcarpet are getting to ♻️ much of their logic. I hope the upcoming contourgeo will be able to do the same.
One additional comment: why did the airfoil baseline change exactly?
src/traces/contour/attributes.js
Outdated
| 'Sets the value or values by which to filter by.', | ||
|
|
||
| 'Values are expected to be in the same type as the data linked', | ||
| 'to *target*.', |
There was a problem hiding this comment.
Hmm. I think this was copied from transforms/filter.js, target makes no sense here.
| v1 = Math.min.apply(null, trace.contours.value); | ||
| v2 = Math.max.apply(null, trace.contours.value); | ||
| v1 = Math.min.apply(null, contours.value); | ||
| v2 = Math.max.apply(null, contours.value); |
There was a problem hiding this comment.
Shouldn't we use Lib.aggNums here in case contours.value has some non-numeric values?
There was a problem hiding this comment.
handleConstraintValueDefaults already cleans this up. Not sure if I actually agree with all the choices made there, but it doesn't look to me as though non-numeric values can get through.
| } | ||
|
|
||
| var cd = heatmapCalc(gd, trace); | ||
| setContours(trace); |
|
|
||
| var constraintMapping = require('./constraint_mapping'); | ||
| var isNumeric = require('fast-isnumeric'); | ||
| var COMPARISON_OPS2 = require('../../constants/filter_ops').COMPARISON_OPS2; |
There was a problem hiding this comment.
Looks like constraint_value_defaults.js is only required in constraint_defaults.js. Would you mind merging them in a single file?
There was a problem hiding this comment.
ah yes, no need for separate files - 7e837ef
| return heatmapHoverPoints(pointData, xval, yval, hovermode, hoverLayer, true); | ||
| var hoverData = heatmapHoverPoints(pointData, xval, yval, hovermode, hoverLayer, true); | ||
|
|
||
| if(hoverData) { |
There was a problem hiding this comment.
Looks like contourcarpet does not have a hoverPoints handler. Could it now reuse this one here?
There was a problem hiding this comment.
I'm guessing it will take some work to sort out hover for contourcarpet. Unlike scattercarpet where you just have to look at the final position of each point, contourcarpet we would actually have to invert (x,y) back to (a,b), which I don't think has been implemented (it's in principle multi-valued anyway).
|
|
||
| 'use strict'; | ||
|
|
||
| module.exports = { |
| var supplyConstraintDefaults = require('./constraint_value_defaults'); | ||
| var addOpacity = require('../../components/color').addOpacity; | ||
|
|
||
| module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout) { |
| }); | ||
| } | ||
|
|
||
| return hoverData; |
There was a problem hiding this comment.
Moverover, should we show hover labels for x/y/z points outside the constraints? I'm not sure what's best here. Maybe that's why Ricky didn't bother implementing hover labels for contourcarpet in the first place.
There was a problem hiding this comment.
One more comment: looks like contours would benefit from a version of hovermode: 'compare' that allows showing multiple labels of super-imposed traces as proposed in #720 (comment)
Currently, hovermode: 'closest' isn't that useful on the contour_constraints mock:
There was a problem hiding this comment.
Moverover, should we show hover labels for x/y/z points outside the constraints?
I think so. In fact it's possible that there actually aren't any data points (only interpolations) within the constraints, for [] type. Anyway I could see people using hover to see "how bad would it be to operate out here"
One more comment: looks like contours would benefit from a version of
hovermode: 'compare'that allows showing multiple labels of super-imposed traces as proposed in #720 (comment)
Yes definitely - but perhaps we tackle that all at once as part of #720? There are going to be some weird cases with contours, like if two constraints overlay but with misaligned grids.
There was a problem hiding this comment.
Yes definitely - but perhaps we tackle that all at once as part of #720?
Absolutely 👌
Because the streamlines and pressure contours weren't supposed to have legend entries at all, they're regular contours, not constraints. I suppose we could make it possible to give legend entries for contour plots with no colorscale, but that's not in scope here. |
|
Looks great! Thanks for clarifying the 💃 |
Extends
contours.type: 'constraint'to regularcontourtraces, not justcontourcarpet.cc @bpostlethwaite - lets not merge until we've waited a bit to see if we need a patch release, but you can use this branch for integration.