Skip to content
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

Fields missing in fullData #2834

Closed
dmt0 opened this issue Jul 23, 2018 · 21 comments
Closed

Fields missing in fullData #2834

dmt0 opened this issue Jul 23, 2018 · 21 comments
Labels
bug something broken

Comments

@dmt0
Copy link
Contributor

dmt0 commented Jul 23, 2018

  1. Missing marker.colorbar.tickfont.color
    https://codepen.io/dmt0/pen/djNNpa?editors=1010 - done in Add missing fields to fullData #2850

  2. For Violin plots missing:

https://codepen.io/dmt0/pen/MBpxLv?editors=1010

  1. For choropleth:
  • marker.line.color
@etpinard
Copy link
Contributor

Missing marker.colorbar.tickfont.color

Yeah, that one sounds like a bug

For Violin plots missing:

bandwidth
span

Those are computed fields. They don't have a fixed default. Their values are punching in gd.calcdata[i][j].t not gd,_fullData. We could copy them back in fullData to make the editor happy, but I'm afraid similar issues are going to come up in other trace types. How does the editor handle other computed values like axis dtick, tick0, histograms xbins etc?

meanline.*

That's the expected behavior. We remove the meanline. container when the violins don't have mean lines:

var meanLineColor = coerce2('meanline.color', lineColor);
var meanLineWidth = coerce2('meanline.width', lineWidth);
var meanLineVisible = coerce('meanline.visible', Boolean(meanLineColor || meanLineWidth));
if(!meanLineVisible) delete traceOut.meanline;

@etpinard
Copy link
Contributor

meanline.*

That's the expected behavior

Oh wait, maybe having gd._fullData[i].meanline.visible = false would have more sense for the editor instead of removing the container completely.

@etpinard
Copy link
Contributor

etpinard commented Jul 24, 2018

... would be nice to talk about these during the weekly meeting on Thursday 📆

@etpinard
Copy link
Contributor

generated pie marker.colors are also missing from fullData: https://codepen.io/veraz/pen/YjxgvE?editors=1010

cc @VeraZab

@etpinard
Copy link
Contributor

Pie marker.colors, violin span, bandwidth all fall under the same category: their defaults are computed during the calc step (which occurs after Plots.supplyDefaults) and reside in gd.caldata, not gd._fullData.

These attributes differ from e.g. contour's contours.(start|end|size) or histogram's (x|y).bins which have their default values computed in calc but punched back into gd.data to preserve the correct autocontours / autobins behavior.

So, I see two possible solutions:

  • Punch in the computed marker.colors, span and bandwidth, write a test in plotly.js and be done with it. This seems like a good solution if we consider marker.colors, span and bandwidth exceptions in the plot-schema. This seems fair as I can't think of any other attribute that behave that way. But we wouldn't be immune to similar problems when adding new trace types.
  • Add a key in the plot-schema (e.g. calcDflt: true) and have the react-chart-editor handle those attributes separately. This key could also be valuable to https://plot.ly/javascript/reference/

About marker.colors, #2205 seems to suggest that punching in the "computed" default into fullData might throw off Plotly.PlotSchema.findArrayAttributes, but I can't remember what were the consequences.

cc @nicolaskruchten @VeraZab @dmt0 @alexcjohnson

@etpinard etpinard added the bug something broken label Jul 26, 2018
@alexcjohnson
Copy link
Collaborator

We can't just fill these into fullData during calc, or Plotly.react will break (ie it will always think these attributes changed, because it's looking at newFullData immediately after supplyDefaults and comparing it to oldFullData after the plot is finished.

A calcDflt schema property might help, though if we do this AND fill the values back into fullData we'll still have a problem figuring out if the value is the default or not - the user inserts a non-default value, then removes it via Plotly.react - we see no value in newFullData and a value in oldFullData, how do we know if that old value is the default or was given explicitly? We could create trace._dflt_bandwidth = false or something...

If we make calcDflt but do NOT fill the value into fullData, how is the editor going to find the value, or even know that it should be expected to look for a value? Doesn't seem like a good idea to force it to reach into calcdata. So I feel like ^^ or something like that would work better.

Pie marker.colors is a tricky one. In addition to the question about findArrayAttributes, subsequent pies can affect each other. If you provide colors for the first pie trace (or even if you don't), any later pies with matching label values will inherit the same color if you don't override it, but previous pies will not. That means even if we manage to pull out the relevant color array, if someone uses that as the base to edit these colors, it could give unexpected results where a user expects to alter the color for all slices of a given label but ends up only altering some of them. We could fix that order dependence, but there would still be an ambiguity then if someone set the color for a certain label in one trace and then changed it in another trace - do they intend both to take the new color, or just one? What if there are 3 pies, you set a color first on the second pie, then you set a different color for the same label on the first pie, then the 3rd will inherit the new color?

Or that's how it's supposed to work anyway... looks like there's a bug where explicit colors don't get propagated correctly, though default colors do. In this test here, the orange 'a' should be red, looks like the mechanism that's supposed to make this work (fullLayout._piecolormap) is getting the wrong key (undefined) somehow:
screen shot 2018-07-30 at 1 54 40 pm

Anyway, aside from making the editor happy, I'm not sure if it matters whether marker.colors defaults are included in fullData. Pie hover events already report the color either way, does it have other meaningful effects? For the editor's sake, seems like we may want to have one (very special) control based on fullLayout._piecolormap that figures out how to set these colors unambiguously, unless we think there's an important reason to support per-trace colors for matching labels.

@nicolaskruchten
Copy link
Contributor

For the editor's sake, seems like we may want to have one (very special) control based on fullLayout._piecolormap that figures out how to set these colors unambiguously

Bingo, I think we just didn't know where that secret key was. /cc @VeraZab this is a more stable approach than my 'copy the colorway' hack :P

@nicolaskruchten
Copy link
Contributor

@alexcjohnson it looks like _fullLayout._piecolormap gives a cross-pie view and we'll have to stitch the actual ordered scale back together... is there some internal function we can use that gives us the colors, in order, for a given pie trace?

@nicolaskruchten
Copy link
Contributor

If not, we may just display the colorway for this particular component if nothing is stored in data... it's "close enough" :)

@etpinard
Copy link
Contributor

Editor devs, is adding violins to the chart editor a high priority?

If so, I'm thinking of simply mutating the computed span and bandwidth default values back into gd._fullData and gd.data similar to how we currently handle histogram bins and contour levels.

@alexcjohnson
Copy link
Collaborator

it looks like _fullLayout._piecolormap gives a cross-pie view and we'll have to stitch the actual ordered scale back together...

yes, that's exactly the reason I suggested it - because that's the only way to unambiguously set the color for a label, without wondering whether it will apply to one, some, or all pies that share that label.

is there some internal function we can use that gives us the colors, in order, for a given pie trace?

No, but you could pull them out of calcdata.

If not, we may just display the colorway for this particular component if nothing is stored in data... it's "close enough" :)

As long as you're not worried about these changes propagating to non-pie traces, and as long as this pie isn't inheriting colors from another one. To me though those seem like pretty substantial caveats!

@nicolaskruchten
Copy link
Contributor

is calcdata available as a reference in fullLayout or fullData somewhere?

@etpinard
Copy link
Contributor

is calcdata available as a reference in fullLayout or fullData somewhere?

no, only on gd.calcdata

@alexcjohnson
Copy link
Collaborator

is calcdata available as a reference in fullLayout or fullData somewhere?

No. And I don't really want to put it there, lest it invite other people to start using it.
Perhaps instead we should put the calculated color array into the fullData trace as a private _calcColors or something?

@nicolaskruchten
Copy link
Contributor

OK thanks. We just figured out how to access calcdata at the point we need it, so we'll just use that :)

@etpinard
Copy link
Contributor

etpinard commented Sep 7, 2018

From @nicolaskruchten 's #2964 on hoverlabel.bgcolor and hoverlabel.font.color:

If I just create any random figure without specifying anything to do with hoverlabel and look at _fullData[0].hoverlabel or _fullLayout.hoverlabel, I see font.family, font.size and namelength but none of the default values for the color keys like bgcolor or bordercolor or font.color. This makes it hard to add these fields to the react-chart-editor.

I expect that the colours for the _fullLayout variant should be null by default, which would mean "delegate to the individual traces" right? That'll be hard to manage in the editor but we'll find a way :)

which I responded by:

In general, we can't just punch the "computed" hoverlabel colors during the defaults (and hence in fullData w/o hacks). One case that comes to mind: a scatter trace with array marker.color will have hover labels that match the marker.color values.

@etpinard
Copy link
Contributor

Pasting in parts of @alexcjohnson 's

#3109 (comment)

it's just a little funny though with valType: 'number' - it means null looks invalid, so if you try and set it we'll determine it to be invalid and fall back on the default, which I guess is fine as long as null is the default, but it would still fail Plotly.validate. Also you can't set it at all via restyle/relayout, because null there means "unset" - again, not immediately a problem as long as this is the default. BUT... what if you have a template that overrides the default? Seems to me then you would be unable to revert to auto with any setting at all of the user attribute, right?

valType: 'angle' adds a special value 'auto', which might be the best of both worlds? Its meaning is clear (and distinct from 0), and it's not intercepted by plotlyjs like null is. Perhaps we should allow valType: 'number' to specify allowAuto: 'true', or extras: ['auto']?

and #3130 (comment)

I have one question about how this will play with RCE (cc @nicolaskruchten @VeraZab) - which currently doesn't seem to support (inside|outside)textfont at all but presumably at some point it will. I'm assuming because insidetextfont.color is undefined by default, we wouldn't show the corresponding color picker - so perhaps we should accept and fill in something like 'auto' instead? (see #3109 (comment) for discussion of null and other options)


I'm starting to feel the nicest way to resolve the remaining violin bandwidth/span problem as well as the trace hoverlabel problem (from the above #2834 (comment)), would be to make -- as @alexcjohnson suggested -- all attributes that have their default value computed downstream of supplyDefaults show up in fullData as 'auto' after supplyDefaults.

Doing this has the potential of solving problems for the RCE, Plotly.validate and templates 🏆

Of course, we can't do this for valType: 'string' attributes, but 'auto' wouldn't never conflict with valid 'number' and 'color' attribute values. The only 'string' attributes I can think of fitting into this category are font family, but looks like from #3027 that shouldn't be a problem. We should double-check though.

@nicolaskruchten
Copy link
Contributor

in principle the proposal above makes sense to me. How much work is it for us to rough this in to test with RCE to see what breaks?

@etpinard
Copy link
Contributor

etpinard commented Nov 8, 2018

How much work is it

~1 day of plotly.js work.

@jackparmer
Copy link
Contributor

This issue has been tagged with NEEDS SPON$OR

A community PR for this feature would certainly be welcome, but our experience is deeper features like this are difficult to complete without the Plotly maintainers leading the effort.

Sponsorship range: $5k-$10k

What Sponsorship includes:

  • Completion of this feature to the Sponsor's satisfaction, in a manner coherent with the rest of the Plotly.js library and API
  • Tests for this feature
  • Long-term support (continued support of this feature in the latest version of Plotly.js)
  • Documentation at plotly.com/javascript
  • Possibility of integrating this feature with Plotly Graphing Libraries (Python, R, F#, Julia, MATLAB, etc)
  • Possibility of integrating this feature with Dash
  • Feature announcement on community.plotly.com with shout out to Sponsor (or can remain anonymous)
  • Gratification of advancing the world's most downloaded, interactive scientific graphing libraries (>50M downloads across supported languages)

Please include the link to this issue when contacting us to discuss.

@gvwilson
Copy link
Contributor

Hi - this issue has been sitting for a while, so as part of our effort to tidy up our public repositories I'm going to close it. If it's still a concern, we'd be grateful if you could open a new issue (with a short reproducible example if appropriate) so that we can add it to our stack. Cheers - @gvwilson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

No branches or pull requests

6 participants