Skip to content
This repository has been archived by the owner on Dec 21, 2023. It is now read-only.

Allow empty plot labels in TC plots #1211

Merged
merged 2 commits into from
Dec 9, 2018
Merged

Allow empty plot labels in TC plots #1211

merged 2 commits into from
Dec 9, 2018

Conversation

znation
Copy link
Contributor

@znation znation commented Nov 13, 2018

Previously, empty string in TC plots was treated as a special
placeholder to indicate default value, and null values weren't
supported in plot APIs below Python.

With this change, null now means to omit the label, empty string
is actually empty string, and there is a __TC_DEFAULT_LABEL
placeholder string to indicate that the default should be used.

Previously, empty string in TC plots was treated as a special
placeholder to indicate default value, and null values weren't
supported in plot APIs below Python.

With this change, null now means to omit the label, empty string
is actually empty string, and there is a __TC_DEFAULT_LABEL
placeholder string to indicate that the default should be used.
Copy link
Contributor

@srikris srikris left a comment

Choose a reason for hiding this comment

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

I've wanted this fix! Love it!

@hoytak
Copy link
Collaborator

hoytak commented Nov 30, 2018

I'm not sure why the default label handling needs to get pushed all the way down to the C++ code. Instead of the special handling of __TURI_DEFAULT_LABEL, which is a bit opaque to the user in python, what if we would do a scheme like

DEFAULT_LABEL = "$(axis)s"

Then in the plot function, do:

xlabel = xlabel % {"axis" : "x"}  # Or column name, whatever. 
ylabel = ylabel % {"axis" : "y"}
...

Then it'd be handled in python and we wouldn't have to special case values.

@znation
Copy link
Contributor Author

znation commented Dec 6, 2018

@hoytak The Python API-level defaults still need to be opaque to the user, since they are different at runtime depending on (for instance) the dtype of the SArray(s). Does that change your opinion of whether I should remove the C++ plumbing for the defaults and move it up to Python? I'm not sure there is any user-facing benefit to that now.

@znation znation merged commit 3d76c50 into apple:master Dec 9, 2018
@znation znation deleted the viz_allow_empty_titles branch December 9, 2018 01:35
@nickjong nickjong added this to the 5.3 milestone Jan 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants