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

Fixed title formatter in GenericElementPlot #4061

Merged
merged 5 commits into from
Oct 27, 2019
Merged

Conversation

poplarShift
Copy link
Collaborator

@poplarShift poplarShift commented Oct 16, 2019

Before:
Screen Shot 2019-10-16 at 2 38 29 PM

After:
Screen Shot 2019-10-16 at 2 46 15 PM

This said, I would also like to point out that the _format_title methods of GenericElementPlot and GenericCompositePlot look pretty similar, and both classes directly derive from DimensionedPlot, yet when both title and title_format are set, one falls back to title, the other one to title_format. I don't know if there was a special reason why the two classes have separate implementations but I think the code could potentially be clearer.

from holoviews import opts
import holoviews as hv
hv.extension('bokeh')
# hv.extension('matplotlib')

l = hv.NdLayout({'FOO': hv.Scatter([], group='ONE'), 'BAR': hv.Scatter([], label='TWO')}, kdims='MYDIM').opts(
    opts.Scatter(title_format='this {label} is {group} my {dimensions} formatter')
)
l

@DancingQuanta
Copy link
Contributor

This is great! I was about to use title_format until I saw this. Is this case already covered in test suite?

@poplarShift
Copy link
Collaborator Author

Hm, I didn't check actually. (I wanted to wait for feedback on whether we should simplify the title/title_format arguments while we're at it but I guess that can be done later too.)

@philippjfr
Copy link
Member

So the idea was that title would eventually replace title_format entirely and they were just aliases. Therefore title should at least for now take precedence. So if we could combine the two implementations in some form and have title take precedence in both cases that would be great.

@poplarShift
Copy link
Collaborator Author

poplarShift commented Oct 22, 2019

Ok I'm having a look at combining the two.

@poplarShift
Copy link
Collaborator Author

Done @philippjfr. MWE to play around with it:

from holoviews import opts
import holoviews as hv
hv.extension('bokeh')

options = [
    opts.Scatter(title='this {label} is {group} my {dimensions} formatter', frame_width=400),
    opts.NdLayout(title='LAYOUT TITLE: this {label} is {group} my {dimensions} formatter'),
    opts.NdOverlay(title='OVERLAY TITLE: this {label} is {group} my {dimensions} formatter')
]
foo = hv.Scatter([], group='ONE', label='first')
bar = hv.Scatter([], group='TWO', label='second')
l = hv.NdLayout({'FOO': foo, 'BAR': bar}, kdims='MYDIM').opts(*options).cols(1).relabel(group='spam', label='eggs')
l

@philippjfr
Copy link
Member

Thanks! Looking much better to me. Any chance of adding some tests?

@philippjfr
Copy link
Member

Oh I'm also happy to start adding a deprecation warning for title_format, maybe leaving it behind a hv.config.future_deprecations flag for now.

@poplarShift
Copy link
Collaborator Author

I'll look at the tests later. I guess we'd need some for a simple Element, a Layout, and an Overlay, and for all backends? (The code should not depend on backend so one could get away with only one I guess.)

@philippjfr
Copy link
Member

(The code should not depend on backend so one could get away with only one I guess.)

That seems fine.

@poplarShift
Copy link
Collaborator Author

I've added tests for the bokeh backend.

@philippjfr philippjfr merged commit f6afa0c into master Oct 27, 2019
@poplarShift poplarShift deleted the title_formatter branch October 31, 2019 02:48
philippjfr pushed a commit that referenced this pull request Nov 5, 2019
* fixed indentation mistake

* moved general part of _format_title into DimensionedPlot

* add deprecation warning

* friendlier deprecation warning

* tests for NdLayout and Element
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.

3 participants