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

Add support for math text glyphs #13612

Merged
merged 3 commits into from
Dec 21, 2023
Merged

Add support for math text glyphs #13612

merged 3 commits into from
Dec 21, 2023

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Dec 20, 2023

This adds support for TeXGlyph and MathMLGlyph, which mirror functionality of TeX and MathML models (thus Glyph suffix), but for vectorized inputs. I also added tex() and mathml() methods to glyph APIs (in figure). From performance perspective this may not be the best implementation for the time being, but at least it guarantees no flickering. Alternatively I would have to go the ImageURL route, which isn't great either.

TeXGlyph supports three display modes:

  • "auto" (the default), where text's values are parsed, requiring TeX delimiters to enclose math content, e.g. $$x^2$$. TeX display mode is inferred by the parser.
  • "block", where text's values are taken verbatim and TeX's block mode is used.
  • "inline", where text's values are taken verbatim and TeX's inline mode is used.

Exmple: (inline, block display, and normal Text glyph for comparison):
image

Python:
import numpy as np

from bokeh.plotting import figure, show

N = 1000
x = np.random.random(size=N) * 100
y = np.random.random(size=N) * 100
radii = np.random.random(size=N) * 1.5
colors = np.array([(r, g, 150) for r, g in zip(50+2*x, 30+2*y)], dtype="uint8")

p = figure()
p.circle(x, y, radius=radii, fill_color=colors, fill_alpha=0.6, line_color=None)

p.text(
    x=[10, 20, 30, 40],
    y=[0, 10, 20, 30],
    text=["AAA", "BBB", "CCC", "DDD"],
    background_fill_color="white", background_fill_alpha=0.8,
    padding=10,
    border_line_color="black",
)

def tex(display, offset, color):
    p.tex(
        x=[10, 20, 30, 40],
        y=[0 + offset, 10 + offset, 20 + offset, 30 + offset],
        text=[
            r"x^2",
            r"\frac{1}{x^2\cdot y}",
            r"\int_{-\infty}^{\infty} \frac{1}{x} dx",
            r"F = G \left( \frac{m_1 m_2}{r^2} \right)",
        ],
        background_fill_color=color, background_fill_alpha=0.8,
        padding=10,
        border_line_color="black",
        display=display,
    )

tex("inline", 20, "yellow")
tex("block", 40, "pink")

show(p)

@mattpap mattpap added this to the 3.4 milestone Dec 20, 2023
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (50cf46c) 92.53% compared to head (2aaa110) 92.54%.

Additional details and impacted files
@@             Coverage Diff             @@
##           branch-3.4   #13612   +/-   ##
===========================================
  Coverage       92.53%   92.54%           
===========================================
  Files             323      323           
  Lines           20476    20493   +17     
===========================================
+ Hits            18948    18965   +17     
  Misses           1528     1528           

@bryevdv
Copy link
Member

bryevdv commented Dec 20, 2023

@mattpap The images and animations you often include in PRs are really great, but FYI I'm usually left wishing the code that generated them was available in order to try things out or experiment with the API.

export {HAreaStep} from "./harea_step"
export {HArea} from "./harea"
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand these re-orderings

In [4]: "HArea" < "HAreaStep"
Out[4]: True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use vim's (or equivalent) :sort command, which is non-semantic, thus it compares S < }.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I resorted this the way eslint would. At some point, when there are fewer open PRs, I will most likely enable relevant lint rules to systematically organize imports and exports.

@bryevdv
Copy link
Member

bryevdv commented Dec 21, 2023

Generally LGTM, a couple of comments/questions:

  • Glyph suffix is "OK" but it's probably a pretty low bar for me to vote for anything else. @tcmetzger any thoughts?
  • @mosc9575 if you happen to have a time, any quick check for docstring syntax preciseness is appreciated

Lastly, folks have at times asked for a unified text API that supports math text inside using $x+y$ etc. Is this us throwing in the towel on that? If not, will these models be redundant at some point in the future?

@mattpap
Copy link
Contributor Author

mattpap commented Dec 21, 2023

Lastly, folks have at times asked for a unified text API that supports math text inside using $x+y$ etc. Is this us throwing in the towel on that? If not, will these models be redundant at some point in the future?

Not necessarily. However, I didn't want to further bloat Text glyph with all the logic specific to handling TeX/MathML/etc. (image loading, async, etc.) and regress its performance. More importantly, we need a cheap way of discovering if bokeh-mathjax.js bundle is needed, which shouldn't require parsing the data, especially when the data may not be available in Python. That may change when dynamic loading of bundles is fully established.

@mattpap
Copy link
Contributor Author

mattpap commented Dec 21, 2023

Glyph suffix is "OK" but it's probably a pretty low bar for me to vote for anything else.

There aren't many possibilities when dealing with naming conflicts:

  1. Use a module related suffix like Glyph in this case.
  2. Use a "creative" name or suffix, e.g. Box suffix in layouts.
  3. Don't expose all models in a flat namespace (my preference per PR Replace data annotations with glyphs #13344).

For the purpose of this PR the first seems to be the simplest to go with, especially that an average user will use figure.tex() most cases.

mattpap and others added 2 commits December 21, 2023 15:05
Co-authored-by: Moritz Schreiber <68053396+mosc9575@users.noreply.github.com>
@bryevdv
Copy link
Member

bryevdv commented Dec 21, 2023

Don't expose all models in a flat namespace (my preference per PR #13344).

OT: We are adding enough new models that it's reasonable to consider. Like I said in the other issue, we just need to have a detailed and specific actual plan that is agreed up front. I would say we have one shot to define a hierarchy structure for models to slot into, so we need to be deliberate about the conceptual basis for that structure, how it might be extended, in the future, how documentation will adapt, etc. And also how to handle a smooth transition period (for at least a few years I would guess).

@mattpap mattpap merged commit 6560e98 into branch-3.4 Dec 21, 2023
28 of 30 checks passed
@mattpap mattpap deleted the mattpap/math_text_glyph branch December 21, 2023 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants