Contour bg fix #1280
Contour bg fix #1280
Conversation
|
|
||
| return propIn.get() ? propOut : false; | ||
| return (valIn !== undefined && valIn !== null) ? propOut : false; |
There was a problem hiding this comment.
@etpinard I included this change bcs contours uses coerce2 on some numeric values that can be 0. This still seems a little weird, ie you have to know to check for exactly false, and if false is an allowed value it still won't be telling you what you think it is. But this fixes it for the exact case I was worried about here.
There was a problem hiding this comment.
Nice.
I've been wanting to merge coerce2 with validate for a while now.
Good enough for now 👍
| role: 'style', | ||
| description: [ | ||
| 'Sets the maximum number of contour levels. The actual number', | ||
| 'of contours will be chosen automatically to be less than or', | ||
| 'equal to the value of `ncontours`.', | ||
| 'Has an effect only if `autocontour` is *true*.' | ||
| 'Has an effect only if `autocontour` is *true* or if', | ||
| '`contours.size` is missing.' |
There was a problem hiding this comment.
So if you specify ncontours and contours: {start, end} we will automatically choose a "nice" size, even though autocontour is false. Any reason not to allow this usage?
Note that the converse is not allowed: you can't specify autocontour=true and use your own contours.size value instead of ncontours - though I suppose it would be clear what this means, as it uses the same machinery as axis ticks, ie it would use a canonical tick of 0 so the contours would be all the multiples of contours.size between the data min and max values. That would actually be kind of nice, maybe I'll add it as well.
There was a problem hiding this comment.
Any reason not to allow this usage?
I can't think of one. Nice addition 👍
Note that the converse is not allowed:
So,
autocontour: true,
contours: {
size: 10
}ignores contours.size, and
autocontour: false,
contours: {
size: 10
}pick contours.start and contours.end using Axes.tickFirst?
There was a problem hiding this comment.
Hmm, I was starting to think autocontour had become completely obsolete, we’d just fill in whatever missing pieces there were. But actually I think I want to make some changes to this:
- with
autocontour=truewe clearstart,end, andsizeand generate them all automatically from the data range andncontours. Perhaps the only reason you'd need to do this is if you previously set some non-auto values and you want to toss them out. - with
autocontour=falsewe still fill in anything that’s missing but won’t ignore anything the user provides. So for example if you only want to specifycontours.endto ignore some outliers, you can do that, you don’t have to also specifycontours.startto get it accepted.
I don't think there's anything backward-incompatible in this proposal, except in partial information cases, where we had a bunch of bugs anyway.
There was a problem hiding this comment.
deferred to #1282
| // pushes its calculated contour size back to the input trace, so for | ||
| // things like restyle that can call supplyDefaults without calc | ||
| // after the initial draw, we can just reuse the previous calculation | ||
| contourSize = coerce('contours.size'), |
There was a problem hiding this comment.
This line - always coercing contours.size rather than only if autocontour is false - is actually all you need to fix #591
| dummyAx.range.reverse(); | ||
| contours.end = Axes.tickFirst(dummyAx); | ||
|
|
||
| if(contours.start === trace.zmin) contours.start += contours.size; | ||
| if(contours.end === trace.zmax) contours.end -= contours.size; | ||
|
|
||
| // so rounding errors don't cause us to miss the last contour | ||
| contours.end += contours.size / 100; |
There was a problem hiding this comment.
this is actually already dealt with in contour.plot, which is easier because we only have to do it once, rather than separately for every edge case we're now going to be checking for here.
| @@ -103,6 +104,11 @@ function emptyPathinfo(contours, plotinfo, cd0) { | |||
| z: cd0.z, | |||
| smoothing: cd0.trace.line.smoothing | |||
| }); | |||
|
|
|||
| if(pathinfo.length > 1000) { | |||
| Lib.warn('Too many contours, clipping at 1000', contours); | |||
There was a problem hiding this comment.
somewhat arbitrary... but consistent with what we do for cartesian ticks - here I included the warning as the consequences (in terms of super slow perf) are likely much more severe and you will want to see the contours object to know why it happened.
| trace._input.contours = extendFlat({}, contours); | ||
| } | ||
| else { | ||
| // sanity checks on manually-supplied start/end/size |
|
💃 looks good to me. |
Fixes #591 and a bunch of other edge cases for contour plots related to
autocontourand / or partial or self-contradictory specification of contour levels.