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

Added Labels element #2352

Merged
merged 13 commits into from
Apr 3, 2018
Merged

Added Labels element #2352

merged 13 commits into from
Apr 3, 2018

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Feb 18, 2018

Adds a Labels element type for vectorized drawing of labels and text. Also adds an emoji clustering dataset (~1 kB in size).

@jlstevens
Copy link
Contributor

I realize this is still WIP but just as a reminder, I would use this PR to add a warning to hv.Text in this PR to say that it will eventually be deprecated.

@philippjfr
Copy link
Member Author

I realize this is still WIP but just as a reminder, I would use this PR to add a warning to hv.Text in this PR to say that it will eventually be deprecated.

I don't necessarily think we have to deprecate Text, Labels and Text are independently useful for different purposes.

@jbednar
Copy link
Member

jbednar commented Apr 3, 2018

The new docs for Labels make it clear how Labels has uses beyond what is supported for Text. If Text has uses not well supported by Labels, presumably that should be made clear now in the docs for Text?

@philippjfr
Copy link
Member Author

Really a matter of convenience, if you want a single label: hv.Text(0, 0, 'Some text') is a lot simpler and more obvious than hv.Labels([(0, 0, 'Some Text')]). I have no strong opinion but I do feel immediately deprecating Text is premature.

@philippjfr
Copy link
Member Author

Ready to review/merge.

"\n",
"**Dependencies**: Bokeh\n",
"\n",
"**Backends**: [Bokeh](./Labels.ipynb), [Matplotlib]('../matplotlib/Labels.ipynb')"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the table that is consistent with all the other element notebooks...

Copy link
Member Author

@philippjfr philippjfr Apr 3, 2018

Choose a reason for hiding this comment

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

That does not format nicely in JupyterLab, all notebooks should be converted to this format. It looks nicer too and gets rid of all the horrible HTML.

Copy link
Contributor

@jlstevens jlstevens Apr 3, 2018

Choose a reason for hiding this comment

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

In that case it should be fixed in a separate PR. Not by mixing different markup.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've used the new format in all the recently added elements, no point reverting those. Happy to follow up this PR by updating all existing reference notebooks (although that should probably wait until your metadata PR is merged).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Sounds good.

"""
Labels represents a collection of text labels associated with 2D
coordinates. Unlike other Annotation types it is vectorized to
draw labels from a dataset.
Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't really an annotation type given that it inherits from chart.Points.

Copy link
Member Author

Choose a reason for hiding this comment

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

True although that's mostly an implementation detail, I can probably get it to inherit from Dataset and Annotation instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, not a good idea due to the custom Annotation.clone

Copy link
Member Author

Choose a reason for hiding this comment

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

Still shouldn't inherit from Points though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not suggesting changing the implementation, just fixing the docstring. Frankly, I see this kind of element as being halfway between a chart and an annotation type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still shouldn't inherit from Points though.

Agreed. But I assume it will have to inherit from some sort of Chart.

Copy link
Member Author

Choose a reason for hiding this comment

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

Dataset+ Element2D is the standard inheritance for something like this. At some point we should probably revisit these baseclasses, e.g. I think Scatter should be a Chart type for instance while Points shouldn't. In my mind Chart describes the 0-1D case of independent vs. dependent dimensions (e.g. Scatter, Curve, Area, Bars etc.), while we've never come up with a baseclass for the 2D space concept that encompasses Points, Path, VectorField etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense - maybe it could be called Chart2D or something. Anyway, I suggest filing an issue with this suggestion if there isn't one already.

@jlstevens
Copy link
Contributor

Implementation looks fine other than few comments I've made above. I still think having so many uses of the word label is annoying but I don't have a better suggestion (don't particularly like MultiText for example). Anyway, happy to merge once my comments are addressed.

@philippjfr
Copy link
Member Author

Ready to merge.

@jlstevens
Copy link
Contributor

Looks good. Merging.

@jlstevens jlstevens merged commit 6d7034d into master Apr 3, 2018
@jbednar
Copy link
Member

jbednar commented Apr 3, 2018

Really a matter of convenience, if you want a single label: hv.Text(0, 0, 'Some text') is a lot simpler and more obvious than hv.Labels([(0, 0, 'Some Text')]). I have no strong opinion but I do feel immediately deprecating Text is premature.

It seems like it would be confusing and ambiguous if hv.Labels() accepted that same signature? If so, then should hv.Text(*a) just become a macro that returns hv.Labels([*a])? I guess the main implication of that would be for options, which would then need to be set on Labels instead of Text. Hmm.

@philippjfr
Copy link
Member Author

I guess the main implication of that would be for options, which would then need to be set on Labels instead of Text. Hmm.

We do have a mechanism for this which is an operation+Compositor, which would look something like this:

class text_to_labels(operation):

    def _process(self, element, key=None):
        params = hv.util.get_param_values(element)
        return Labels([(element.x, element.y,  element.text)], **params)

Compositor.register(Compositor("Text", text_to_labels, None,
                               'data', transfer_options=True,
                               transfer_parameters=True,
                               output_type=Labels))

@jbednar
Copy link
Member

jbednar commented Apr 3, 2018

@philippjfr, that sounds like a reasonable approach. hv.Text can then be introduced as a simple convenience wrapper for Labels...

@philippjfr philippjfr deleted the labels_element branch November 12, 2018 17:59
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants