Skip to content

Add custom_data argument to px functions #1764

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

Merged
merged 22 commits into from
Sep 12, 2019
Merged

Add custom_data argument to px functions #1764

merged 22 commits into from
Sep 12, 2019

Conversation

emmanuelle
Copy link
Contributor

@emmanuelle emmanuelle commented Sep 10, 2019

closes #1751

@emmanuelle
Copy link
Contributor Author

The way it works:

  • the index is always added to customdata of the trace
  • then the columns specified in custom_data are added to customdata
  • then the ones of hover_data
  • the hover template uses the only the indices of custom_data provided by hover_data

I did not implement deduplication because this will behave correctly even if the user provides the same column in custom_data and hover_data, (at the cost of unnecessary data copy).

@nicolaskruchten
Copy link
Contributor

How hard would it be to implement deduplication?

@nicolaskruchten
Copy link
Contributor

basically it would be a matter of, when processing hover_data, to see if the data's already there... I think we already do something like this in this step in case the data is already in x or y or something

@emmanuelle
Copy link
Contributor Author

@nicolaskruchten no it wouldn't be hard, I can do it if you prefer. Much easier than debugging the CI problem with chart_studio anyway :-/. At the moment I'm adding nottest decorators to isolate the failing tests but of course it is only temporary

@nicolaskruchten
Copy link
Contributor

hehe ok why don't you add the dedupe code and I'll try to crack this Chart Studio thing :)

This reverts commit 466abf7.
This reverts commit b39e460.
@emmanuelle
Copy link
Contributor Author

Regarding deduplication, on second thoughts it might be confusing to users. Let's say a user supplies custom_data=['sepal_length', 'sepal_width'] and hover_data=['petal_length', 'sepal_width']. In a Dash callback, they would expect to access sepal_width at customdata[2] (2 and not 1 because of the index, which is already confusing...). But if we remove duplicates but sill want to preserve the order of hover items then the index will change. I''ll look for a workaround but not sure there is one. I'm also a bit worried by the fact that if customdata[0] is the index it changes the indices of the user-provided columns. Should index be instead customdata[-1] (quite different from the Pandas convention of course, but maybe less confusing).

@nicolaskruchten
Copy link
Contributor

So with custom_data=['A', 'B'] and hover_data=['C', 'B'] I would expect the items in customdata to be [<index>, A, B, C] but the order in the hoverlabel to remain C, B... this seems doable, no?

@nicolaskruchten
Copy link
Contributor

I agree that putting the index in the 0-position shifts things over 1 and makes things a bit confusing, but the value of having the index always available in the same position is pretty high.

@emmanuelle
Copy link
Contributor Author

this is why I suggested -1, which can be argued to be always at the same position, starting from the end.

@emmanuelle
Copy link
Contributor Author

I wonder which solution (index at 0 and then increment other positions by 1, or keep positions and have index at -1) will confuse users less... Any thoughts @jonmmease ?

@emmanuelle
Copy link
Contributor Author

and yes what you suggested is doable.

@nicolaskruchten
Copy link
Contributor

Really in an ideal world we would use the ids attribute, but that has special meaning in the context of animations :(

My worry about putting it in the last position is that it's easy for a user to say "oh it's here in position N" and then hardcode N in an app instead of hardcoding it as -1 or 0.

@nicolaskruchten
Copy link
Contributor

Come to think of it, I don't think there's a restriction on the type of customdata so we could store an object in there like {"index_value": "whatever", "data": ["bla", "bli", "blo"]}

@emmanuelle
Copy link
Contributor Author

Deduplication done. As soon as the percy PR is merged I'll rebase to compare, and then we can converge on the format of customdata.

@nicolaskruchten
Copy link
Contributor

How about this: let's just not automatically insert the index!

Once #1767 is done, the user can simply say custom_data=[df.index] and put it wherever they like, if they like :)

@emmanuelle
Copy link
Contributor Author

ok I removed the index. This should be ready, I'll work on a demo now.

----------
args : dict
args to be used for the trace
trace_spec : dict
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a NamedTuple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@nicolaskruchten
Copy link
Contributor

Code looks fine, but this is probably a case where a simple test case would be nice, especially during review, for stuff that's invisible like this :)

@@ -111,6 +111,10 @@
colref_list,
"Values from these columns appear as extra data in the hover tooltip.",
],
custom_data=[
colref_list,
"Values from these columns are extra data, to be used in widgets or Dash callbacks for example.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a note that this data is not user-visible, but is included in events emitted by the figure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@emmanuelle
Copy link
Contributor Author

@nicolaskruchten about a simple test, definitely yes. I still need to work on #1754 there were some version conflicts in particular to be solved.

@emmanuelle
Copy link
Contributor Author

See https://gist.github.com/emmanuelle/fba6f0a2c387100ac1cc027b4b37e20a for a Dash app showcasing the use of custom_data.

@emmanuelle
Copy link
Contributor Author

The good thing is that Percy did not detect changes.

@nicolaskruchten
Copy link
Contributor

Great! Yeah Percy is pretty good, sometimes throws false positives but not enough to want to ditch it :)

So next steps on this PR are to land #1754 onto master, then rebase this branch onto it, add a test and merge it?

@emmanuelle
Copy link
Contributor Author

yes ! #1754 might be a bit tricky because of version management / conflicts, but let's see how it goes.

@nicolaskruchten
Copy link
Contributor

💃 very nice, thank you :)

@emmanuelle emmanuelle merged commit b770468 into master Sep 12, 2019
@emmanuelle emmanuelle deleted the custom_data branch September 12, 2019 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

px custom_data argument
2 participants