Skip to content

Add support for custom scatter markers #14165

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 3 commits into from
Dec 3, 2024
Merged

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Nov 25, 2024

This PR allows to define custom scatter markers using Scatter(defs={"@new_name": CustomJS(...)}). Custom marker names are prefixed with @, so that they can be differentiated easily (and on the type level) from built-in markers.

From examples/basic/scatters/custom_markers.py:

image

@bryevdv
Copy link
Member

bryevdv commented Nov 25, 2024

Just noting this exact ask came up on the Discourse recently: https://discourse.bokeh.org/t/custom-markers-extensions-and-registering-new-marker-types/12089/2

@bryevdv
Copy link
Member

bryevdv commented Nov 25, 2024

I had imagined this with a dedicated CustomJSMarker but it we can get by with existing CustomJS maybe that's fine with enough documentation support.

I do think passing the defs to individual scatter calls is a little awkward. At least personally, I'd rather register them explicitly up front to use later.

custom_markers = {"@foo": ..., "@bar": ...}

Scatter.register(custom_markers)

plot.scatter(x, y, marker="@foo")
plot.scatter(x, y, marker="@bar")

I'm sure there are other possibilities, e.g. decorator or something.

@mattpap mattpap force-pushed the mattpap/custom_markers branch from 86fe872 to d8328c1 Compare November 25, 2024 23:19
@mattpap
Copy link
Contributor Author

mattpap commented Nov 25, 2024

I had imagined this with a dedicated CustomJSMarker but it we can get by with existing CustomJS maybe that's fine with enough documentation support.

I'm considering adding CustomJSMarker for the sake of documentation, though it would still inherit from CustomJS. For that, however, I need override support implemented in PR #13344.

I do think passing the defs to individual scatter calls is a little awkward. At least personally, I'd rather register them explicitly up front to use later.

If there's only one .scatter() call then the implemented API should be sufficient. When multiple glyphs are involved, then having preregistered markers would be beneficial. I need to think where those definitions would live, because currently we don't have a mechanism for storing "static" data and models.

@bryevdv
Copy link
Member

bryevdv commented Nov 25, 2024

because currently we don't have a mechanism for storing "static" data and models.

Yup I expected that to be the main little roadblock. It's something that could have been useful in other contexts though so using this as a motivating use-case might be worthwhile.

@mattpap
Copy link
Contributor Author

mattpap commented Nov 26, 2024

Yup I expected that to be the main little roadblock. It's something that could have been useful in other contexts though so using this as a motivating use-case might be worthwhile.

Technically we already have something quite similar to this for Document and custom model definitions, but that would have to be expanded in scope and adapted. I will start a new feature issue for this.

@mattpap mattpap force-pushed the mattpap/custom_markers branch from 8ff1898 to d07ce80 Compare December 3, 2024 20:26
@mattpap mattpap requested a review from bryevdv December 3, 2024 20:27
Copy link
Member

@bryevdv bryevdv left a comment

Choose a reason for hiding this comment

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

Simulating positional args seems a bit unconventional in TypeScript. FWIW I think splitting into apply and apply_for_path or something similar would also be reasonable.

@mattpap
Copy link
Contributor Author

mattpap commented Dec 3, 2024

Simulating positional args seems a bit unconventional in TypeScript. FWIW I think splitting into apply and apply_for_path or something similar would also be reasonable.

This is standard practice in JavaScript, thus it's standard practice TypeScript. TypeScript covers existing patterns in JavaScript and it isn't (as much as possible) opinionated about chosen patterns. This also follows the canvas API. I would typically split such API into multiple methods if signatures were too complex and/or there were run-time performance implications of a unified API.

@mattpap mattpap merged commit 9948be0 into branch-3.7 Dec 3, 2024
25 checks passed
@mattpap mattpap deleted the mattpap/custom_markers branch December 3, 2024 23:11
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 Mar 19, 2025
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.

2 participants