Add ability to rename grouped traces #1919

rreusser · 2017-07-27T22:23:22Z

This PR adds the ability to rename grouped traces. The logic is as follows:

provides nameformat as a groupby setting. The default is %g (%t), which expands out into, for example, "groupName (traceName)". (This part is really easy to change. Feedback welcome.)
You can provide overrides in styles

The result is that you can apply intelligent naming and also have the ability to override expanded trace names by modifying the legend.

To accomplish the styles part, I've implemented Lib.keyedContainer, which manages modifying and constructing updates for objects like [{name: ..., value: ...}], which are becoming numerous and kind of unwieldy to manage.

As a result of the discussion below, the syntax for naming groups is now integrated with styles:

{
  type: 'groupby',
  styles: [{
    target: 'a',
    value: {name: 'foo'}
  }]
}

To do:

I've left groupnames as type: "any" because I need to figure out how to make this a properly nested container array.
I'd love it if the groupName (traceName) format were only applied when there's more than one trace, but, unless I'm mistaken, we'd need to expose a bit more global state to transform supplyDefaults, which needs just a bit of thought to make sure we really can't avoid it. 😬
Tests. (Holding off until confirmed that this is a good approach).

/cc @etpinard @alexcjohnson

rreusser · 2017-07-27T22:30:26Z

On usage of the fancy container array thing. It's basically used like this:

var carr = Lib.keyedContainer(fullInput, 'transforms[' + i + '].groupnames');

if(BLANK_STRING_REGEX.test(text)) {
  carr.remove(legendItem.trace._group);
} else {
  carr.set(legendItem.trace._group, [text]);
}

Plotly.restyle(gd, carr.constructUpdate(), [traceNumber]);

When you add keys, it constructs the update. If you remove a key, it slides over all the other indices to keep the storage compressed (rather than a thousand undefined's because you kept modifying the plot).

Really, the whole goal is just to keep things schema-conforming without arbitrary keys. That may or may not be the right approach here, but this library function at least facilitates the process.

rreusser · 2017-07-28T02:28:07Z

I've made my own requested changes to this, including internal cleanup and that it now shows the "groupName" if there's only one trace or "groupName (traceName)" if there's more than one trace. I had to pass totalTraceCount all the way through to transform supplyDefaults to accomplish this, which is ugly, though it does work. Very open to feedback on any of this PR (including just scrapping/rethinking it).

etpinard · 2017-07-28T19:33:59Z

src/components/legend/draw.js

+                    update[direction + '.name'] = text;
+                } else if(fullInput.transforms) {
+                    for(i = fullInput.transforms.length - 1; i >= 0; i--) {
+                        if(fullInput.transforms[i].type === 'groupby') {


Might be nice to add a lib function similar to Registry.traceIs for transforms e.g. Registry,hasTransform('groupby')

Added Regsitry.hasTransform and Registry.getTransformIndices to accomplish this. It's less efficient than the above code, but so marginally that I think it's fine. ✅

etpinard · 2017-07-28T19:35:33Z

src/components/legend/draw.js

+                // string is retained.
+                if(needsRedraw) {
+                    p = p.then(function() {
+                        return Plotly.redraw(gd);


Hmm. We shouldn't have to do this. We should fix _restyle so that it leads to the correct update.

Agreed. I'll try to make that happen.

I'm having trouble reproducing that this was ever necessary. It seems to update just fine. Giving this the 🔪 and I think it's fine.

etpinard · 2017-07-28T19:36:33Z

src/lib/keyed_container.js

+var VALUE = 2;
+var BOTH = 3;
+
+module.exports = function keyedContainer(baseObj, path, keyName, valueName) {


Interesting. I like it.

@rreusser can you of other situations where this could be useful?

Yeah. It's also used in group styles, for one, though that case might be slightly more complicated. Also frames, though again, slightly more complicated. It's more a matter of realizing that unless we allow custom keys in the schema, abstracting this pattern seems like the only sane way forward.

etpinard · 2017-07-28T19:39:09Z

src/plots/plots.js

@@ -798,7 +798,7 @@ plots.supplyDataDefaults = function(dataIn, dataOut, layout, fullLayout) {

    for(i = 0; i < dataIn.length; i++) {
        trace = dataIn[i];
-        fullTrace = plots.supplyTraceDefaults(trace, cnt, fullLayout, i);
+        fullTrace = plots.supplyTraceDefaults(trace, cnt, fullLayout, i, dataIn.length);


I would prefer stashing dataIn.length on fullLayout then passing it around like this.

Something like fullLayout._dataInLength that could be set here side-by-side to fullLayout._dataLength.

Sounds good.

Oh, actually layout._dataLength is the length of the input data that I'm looking for. I think it's best not to make the count depend upon visibility (legend shouldn't fundamentally change when you toggle visibility, right?), so I think it's best to just use layout._dataLength for this.

etpinard · 2017-07-28T19:42:00Z

src/plots/plots.js

+
+                // The group key gets cleared. If set, pass it forward
+                if(expandedTrace._group) {
+                    fullExpandedTrace._group = expandedTrace._group;


Hmm. That's too bad. Do we really need this _group, i.e. could just recompute it during Legend.draw?

Or maybe we could call relinkPrivateKeys here, to make this solution generalizable?

Yeah I didn't like it. I'll try to clean it up.

Switched to relinkPrivateKeys(fullExpandedTrace, expandedTrace), which seems to me like probably the most general approach here as long as it's not too expensive.

etpinard · 2017-07-28T19:49:39Z

src/transforms/groupby.js

@@ -35,6 +35,35 @@ exports.attributes = {
            'with `x` [1, 3] and one trace with `x` [2, 4].'
        ].join(' ')
    },
+    nameformat: {


Bringing in @alexcjohnson @cldougl @chriddyp @cpsievert @monfera what do you think of this syntax?

A lot of existing attributes could benefit form special formatting character like this one. For example,

trace = { y: [1, 2, 1] marker: { size: [20, 30, 10] }, hovertext: '%marker.size' }

would be a nice shortcut to show each point's marker.size value on hover.

Personally, I like %. Though maybe I'd vote for enforcing it with {} e.g. %{name} similar to ES6 ${name}.

... and I would make user spell out %{name} and %{group} as opposed to e.g. %t and %g

I really like the %{group} format.

Yeah, this is cool! I hadn't thought about extending this to other keys but I can imagine that going off in all sorts of directions in the future (%{marker.size*5}???).

%{...} seems sufficiently useless on its own that we can do this without calling it a breaking change.

Ah, I see. Like a general substitution framework? Yeah, the goal here was just a one-off custom substitution in which it only knows about a couple properties. But definitely possibilities. The only thing then would be choosing awkward names like %{group} and %{name} (instead of %{trace}) so that it's backward compatibility in case we substitute trace properties instead of special/custom/ad hoc keys.

@etpinard @alexcjohnson I've added Lib.templateString that performs these substitutions. At the moment it just grabs object properties if it's simple or uses nestedProperty if the key is more complicated than just a plain word. For this case the only data available is %{trace} and %{group}, but it at least opens the door to a unified interface for taking this further (or worst-case-scenario it's only a couple lines of code if it's not more generally useful). Feedback welcome.

See:

plotly.js/src/lib/index.js

Line 748 in 57dfe4a

lib.templateString = function(string, obj) {

rreusser · 2017-08-01T22:38:53Z

I've made the requested changes and have added a test for editing the legend that includes regular and grouped traces. I don't currently have anything further to add.

etpinard · 2017-08-02T18:19:28Z

test/jasmine/tests/register_test.js

@@ -277,4 +277,54 @@ describe('the register function', function() {

        expect(Registry.transformsRegistry['mah-transform']).toBeDefined();
    });
+
+    describe('getTransformIndices', function() {


thanks for the tests!

etpinard · 2017-08-02T18:19:43Z

src/components/legend/draw.js

-                }
-
-                return p;
+                return Plotly.restyle(gd, update, traceIndex);


etpinard · 2017-08-02T18:20:09Z

src/transforms/groupby.js

@@ -113,7 +113,7 @@ exports.supplyDefaults = function(transformIn, traceOut, layout, traceIn, inputT
    if(!enabled) return transformOut;

    coerce('groups');
-    coerce('nameformat', inputTraceCount > 1 ? '%g (%t)' : '%g');
+    coerce('nameformat', layout._dataLength > 1 ? '%g (%t)' : '%g');


This makes me very happy.

etpinard · 2017-08-02T18:22:50Z

src/transforms/groupby.js

-            'with trace name (`"%g (%t)"`). Available escape sequences are `%g`, which',
-            'inserts the group name, and `%t`, which inserts the trace name. If grouping',
+            'defaults to the group name (`"%{group}"`), otherwise defaults to the group name',
+            'with trace name (`"%{group} (%{trace})"`). Available escape sequences are `%{group}`, which',


Shouldn't this be %{name} instead of %{trace} so that it corresponds to an attribute just like group?

... but wait, group isn't a attribute either (groups is though). So how should we do this?

Yeah, you're right. There are two options here:

Expose special sequences. That's what I've done here. So only group and trace are available. They're custom.

Make this a more general pattern of evaluating trace properties, in which case it would indeed be %{name} and… well, there's not really a property for the group unless we add it custom which is why I went with 1 here.

Though to be honest, I'm starting to think we should intercept %{group} and %{name} for this special case but also allow more general evaluation.

I'm starting to think we should intercept %{group} and %{name} for this special case

I'm starting to think the same. 👍 for the current implementation.

One small change though, right? Perhaps templateString should accept multiple objects from which to pull values and it takes the first one with a value. Then group and name would be the first preference before grabbing trace properties. Specifically:

Now:

Lib.templateString('%{group} (%{trace})', {group: ..., trace: ...})

Proposed:

Lib.templateString('%{group} (%{name})', {group: ..., name: ...}, traceObj);

So that technically %{marker.size} would be equally valid.

This plural vs singular problem came up too in #1770 ➡️ here - where values of plural attributes are listed using a singular (grammatically) key.

Maybe we should start declaring corresponding singular key in the plot schema and have Lib.templateString and appendArrayPointValue use it.

Interesting. The only tricky part is that to be really precise, it would have to be %{transforms[2].group}, which it then links up with the schema to realize the pluralized key is in the schema and is a data array, and can be indexed and expanded to %{transforms[2].groups[12]}. The problem I see with that is that templateString must then receive the schema and an item index and that the key must contain the transform number, both of which make it harder to implement and use. Correct me if I'm mistaken though.

etpinard · 2017-08-02T18:35:20Z

src/plots/plots.js

-                if(expandedTrace._group) {
-                    fullExpandedTrace._group = expandedTrace._group;
-                }
+                relinkPrivateKeys(fullExpandedTrace, expandedTrace);


Maybe add a comment here e.g.:

relink private (i.e. underscore) keys expanded trace to full expanded trace so that transform supply-default methods can set _ keys for future use.

etpinard · 2017-08-02T18:37:11Z

test/jasmine/tests/legend_test.js

+            }).then(delay(20));
+        }
+
+        it('sets and unsets trace group names', function(done) {


Lovely test.

It's testing a fair amount of feature actions which isn't the best, but it's not bad for end-to-end.

etpinard · 2017-08-07T14:53:43Z

@rreusser any updates on this? My #1919 (comment) still stands as of today.

We don't have to implement the proposed full attribute-value-to-text-replacement scheme in this PR.

rreusser · 2017-08-07T17:19:30Z

Sounds good @etpinard. Can you clarify that the comment you're referring to is that %{group} and %{trace} seem reasonable even though they're not attributes? Does that conflict with future-proofing this in case we do make them attributes? In other words, I'm just worried that if we make them special cases now, we'll never be able to remove them.

etpinard · 2017-08-07T17:43:16Z

that %{group} and %{trace} seem reasonable even though they're not attributes?

Exactly.

Does that conflict with future-proofing this in case we do make them attributes?

%{trace} should never conflict (having attributes named trace in trace object would be very 😕 )

I guess %{group} could conflict with e.g. %{transforms[i].groups[j] (not sure what the indices mean here), but to me, it's definitively a special that we should expose.

rreusser · 2017-08-07T18:40:12Z

Thanks for clarifying @etpinard. If that's the case, then I think this is complete. You can see here that group and trace are currently the only attributes exposed, though templateString is in principle more powerful and its use could be expanded (though if not, it's only a couple lines of code anyway so not a big deal).

rreusser · 2017-08-07T18:50:45Z

Confirming 💃ability then, @etpinard / @alexcjohnson ?

etpinard · 2017-08-07T21:17:17Z

src/transforms/groupby.js

+            'default "%{group} (%{trace})" would return "Monaco (GDP per capita)".'
+        ].join(' ')
+    },
+    groupnames: {


Hmm. I missed this during my first review. I'm not a fan of groupnames being an array container.

We could instead declare groupnames as a data_array and expecting its length to be equal to groups.length, but that would lead to some (maybe a lot of) duplication.

Maybe we could groupname attribute to the styles container below instead which already expects a target group?

Actually, do we even need a groupnames attribute? groups items can be any strings, so setting groupnames[i].name is equivalent to changing the corresponding groups item.

I apologize for not catching this sooner.

setting groupnames[i].name is equivalent to changing the corresponding groups item.

Not quite - the name will be set by nameformat - ie a combination of the group value and the trace name by default - but when you want to override that (perhaps by typing something specific in the editable legend) you'll need this.

We could instead declare groupnames as a data_array and expecting its length to be equal to groups.length, but that would lead to some (maybe a lot of) duplication.

Or ambiguity, if the names were not perfectly duplicated. What would that mean?

Maybe we could groupname attribute to the styles container below instead which already expects a target group?

That sounds great! so each entry in styles would have target and optionally value and/or name? No need to have two separate containers both keyed on the group value.

The main problem is that groups have no correspondence with groupnames, which I think rules out a plain array for the API. I think there's a good argument to be made though for merging styles with groupnames. Actually in hindsight, that does seem best to me. Not sure why I didn't realize that sooner…

That sounds great! so each entry in styles would have target and optionally value and/or name?

Exactly. I'd vote for name over value, but I don't have a strong opinion here.

value is all the other styles... actually, would it work to just put name inside value, and not have to create any new API? We'd still of course have to point the editing routine at styles[i].value.name but seems like that's all, right?

I think that's the best option. 👍 Then it really is just a style property (which could be set anyway and in fact we'd be doing something surprising not to use what's already possible).

FWIW, IMO, TBH, I'm even happier now that I took the time to implement keyedContainer…

would it work to just put name inside value

good call 👍

etpinard · 2017-08-15T14:18:26Z

src/lib/keyed_container.js

+            // If it's not an unset, force that bit to be unset. This is all related to the fact
+            // that undefined and null are a bit specially implemented in nestedProperties.
+            if(value !== null) {
+                changeType = changeType & ~UNSET;


What does & ~4 do?

I'm tracking what needs to be updated with a bit mask (which, before adding a bit to track whether it's unset in a way that nestedProperty needs to handle specially, originally made 10% more sense since everything else was strictly an OR operation). & ~4 unsets precisely the 4 bit.

etpinard · 2017-08-15T14:26:15Z

Well-earned 💃 @rreusser

Before merging, would you mind shedding some light about those bitwise operations (in a PR comment or in the code directly, whatever you prefer). Also, could you update the PR header (which still mentions groupnames) for us potentially looking over this PR again in the future?

rreusser · 2017-08-15T19:34:20Z

I'm having second thoughts about storing name inside styles because removing styles is how the UI reverts a group's styles to the defaults. That means reverting to default styles will unset the name, which suggests the nesting is a bit incorrect. I think it might be better to store the name next to the styles, which would then allow them to operate separately. Long story short, managing these container array objects in any general sense is rather more convoluted than it first appears. 😕 /cc @alexcjohnson @etpinard

rreusser · 2017-08-15T19:46:09Z

After a brief offline conversation with @alexcjohnson, I have to agree that the current API seems best and that it should be a concern of, for example, the UI to construct the proper update that doesn't unset the name accidentally. With that then, I'm content.

Below is the last case that was rather tricky. If you remove styles items or slide them over to fill unused gaps in the container array, the marker size accidentally gets dropped. I fixed it by just leaving unused container in place rather than compressing the group styles.

alexcjohnson · 2017-08-15T20:36:30Z

src/components/legend/draw.js


-                    astr = direction + '.name';
+                    if(BLANK_STRING_REGEX.test(origText)) {


hmm, there is a use case for a blank name - maybe less so for groupby than for regular traces, but sometimes you want the title for one trace to stand in for a few traces below it. You could argue then that this should just be testing for an actual empty string '' but a space ' ' should be allowed as the name.

okay, I could just check === '' instead.

alexcjohnson · 2017-08-15T21:58:12Z

💃 (again 😄)

rreusser · 2017-08-15T22:07:05Z

Okay, merging this at last. This API seems correct this way, though it makes the updates a bit tricky to compute. Improvements like compressing unused groups out of the picture would be a nice, backwards-compatible enhancement.

Add ability to rename grouped traces

aef38e5

rreusser added 6 commits July 27, 2017 15:47

Fix l egend naming

d886c9e

Rename namepattern --> nameformat

e6f5e13

Remove unused mock

7a2316d

Add custom prop names to keyedContainer

4e6a0aa

Fix groupby groupnames supplyDefaults

952e4ea

Make groupby nameformat dependent on number of traces

8f37fc7

rreusser added the status: reviewable label Jul 28, 2017

etpinard reviewed Jul 28, 2017

View reviewed changes

etpinard added the feature something new label Jul 31, 2017

rreusser added 6 commits August 1, 2017 13:05

Add registry methods for transforms

dd035ef

Cut out unnecessary redraw in legend update

7298a0c

Remove unneeded inputTraceCount parameter

64d2bcd

Add templateString method to perform substitutions

57dfe4a

Replace ad hoc solution with relinkPrivateKeys

c0089ea

Add test for editing legend

42d76e0

etpinard reviewed Aug 2, 2017

View reviewed changes

src/components/legend/draw.js Outdated

}

return p;

return Plotly.restyle(gd, update, traceIndex);

Copy link

Contributor

etpinard Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fun 👍

etpinard reviewed Aug 2, 2017

View reviewed changes

Add comment for relinkPrivateKeys + transforms

efcef43

etpinard added this to the v1.30.0 milestone Aug 4, 2017

etpinard reviewed Aug 7, 2017

View reviewed changes

rreusser added 3 commits August 8, 2017 12:15

Enhance keyedContainer to accept nested props

daac59d

Fix group -> target in groupby styles

6aefb41

Extend keyedContainer to handle nested keys

be0e693

rreusser force-pushed the rename-grouped-traces branch from 9c4c4b3 to be0e693 Compare August 15, 2017 00:47

etpinard reviewed Aug 15, 2017

View reviewed changes

Fix keyedContainer behavior to unset entries without removing

4943e3a

alexcjohnson reviewed Aug 15, 2017

View reviewed changes

Only blank string unsets legend items

4b04131

rreusser merged commit 4ae95dd into master Aug 15, 2017

rreusser deleted the rename-grouped-traces branch August 16, 2017 19:48

alexcjohnson mentioned this pull request Sep 19, 2017

Legend toggles visibility of all groupby groups #2013

Closed

etpinard mentioned this pull request Sep 13, 2018

Template strings in hover data #3007

Closed


		astr = direction + '.name';
		if(BLANK_STRING_REGEX.test(origText)) {

Add ability to rename grouped traces #1919

Add ability to rename grouped traces #1919

Conversation

rreusser commented Jul 27, 2017 • edited Loading

To do:

rreusser commented Jul 27, 2017

rreusser commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser commented Aug 1, 2017 • edited Loading

etpinard Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Aug 7, 2017 • edited Loading

rreusser commented Aug 7, 2017 • edited Loading

etpinard commented Aug 7, 2017

rreusser commented Aug 7, 2017 • edited Loading

rreusser commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Aug 15, 2017

rreusser commented Aug 15, 2017 • edited Loading

rreusser commented Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Aug 15, 2017

rreusser commented Aug 15, 2017

rreusser commented Jul 27, 2017 •

edited

Loading

etpinard Jul 28, 2017 •

edited

Loading

rreusser Aug 1, 2017 •

edited

Loading

rreusser commented Aug 1, 2017 •

edited

Loading

etpinard Aug 2, 2017 •

edited

Loading

rreusser Aug 2, 2017 •

edited

Loading

rreusser Aug 2, 2017 •

edited

Loading

rreusser Aug 2, 2017 •

edited

Loading

etpinard commented Aug 7, 2017 •

edited

Loading

rreusser commented Aug 7, 2017 •

edited

Loading

rreusser commented Aug 7, 2017 •

edited

Loading

rreusser Aug 7, 2017 •

edited

Loading

rreusser Aug 7, 2017 •

edited

Loading

etpinard Aug 15, 2017 •

edited

Loading

rreusser commented Aug 15, 2017 •

edited

Loading

rreusser commented Aug 15, 2017 •

edited

Loading