Deprecate `TexMobject` and `TextMobject` in favour of `MathTex` and `Tex` #391

Aathish04 · 2020-08-31T13:36:54Z

List of Changes

Rename TextMobject to Tex
Replace all instances of TexMobject with MathTex and TextMobject with Tex.
Get rid of single-use TexSymbol.
Rename SingleStringTexMobject to SingleStringMathTex.
Add Deprecation warnings to TexMobject and TextMobject.
Edit __all__ for tex_mobject.py

Motivation

The names TextMobject and TexMobject are incredibly confusing to new users owing to the similarity in their names and lack of information differentiating them and the Text object which are wholly unrelated.

Renaming the objects to be more specific would be very beneficial, especially to new users.

Explanation for Changes

Changes include renaming the objects as suggested, and replacing all instances of the old names with the new ones.

To ease migration from 3b1b/manim, instead of outright removing Tex[t]Mobject, deprecation warnings have been added instead.

Testing Status

Tests currently fail. I (or anyone else interested) will port the tests to the new names once #335 is merged as it refactors the testing mechanism.

Further Comments

See discord discussion starting here.

Acknowledgement

I have read the Contributing Guidelines

Rename TextMobject to Tex Replace all instances of TexMobject with MathTex and TextMobject with Tex. Get rid of single-use TexSymbol. Rename SingleStringTexMobject to SingleStringMathTex. Add Deprecation warnings to TexMobject and TextMobject.

manim/mobject/svg/tex_mobject.py

leotrs · 2020-08-31T13:47:14Z

manim/mobject/svg/tex_mobject.py

+class Tex(MathTex):
    CONFIG = {
        "alignment": "\\centering",
        "arg_separator": "",
        "type": "text",
    }


Comment (non-blocking): What never made sense to me is that I would expect MathTex() to be equivalent to something like Tex(mathmode=True) or something like that. But it seems to be the other way around, and you can't even really make one from the other. But anyway.

we should probably change this at some point

leotrs · 2020-08-31T13:47:51Z

SGTM. So the change is TexMobject -> Tex and TextMobject -> MathTex?

kilacoda-old · 2020-08-31T13:49:37Z

I'd prefer to have Tex[t]Mobject as it is, and have Tex and MathTex as aliases, as follows:

Tex = TextMobject
MathTex = TexMobject

Aathish04 · 2020-08-31T13:49:51Z

SGTM. So the change is TexMobject -> Tex and TextMobject -> MathTex?

No, exactly the other way round.

TexMobject -> MathTex, since that's what uses LaTeX MathMode. TextMobject -> Tex since it's just normal non-maths-y LaTeX.

Aathish04 · 2020-08-31T13:50:32Z

I'd prefer to have Tex[t]Mobject as it is, and have Tex and MathTex as aliases, as follows:
Tex = TextMobject
MathTex = TexMobject

Why though? I was thinking we could remove those horribly confusing names entirely, but keep them for the time being so people can migrate.

leotrs · 2020-08-31T13:57:59Z

If you just rename them then there is no way of issuing a deprecation warning. I agree they should go away in v2.0

kilacoda-old · 2020-08-31T14:00:19Z

Because many people are already used to these names, and understand their differences, and I don't feel it's justified to force change onto them by breaking their old code. By adding aliases, and perhaps a little one-liner note in the docstrings, we can solve the problem of confusing names without breaking peoples' existing code.

Also, TexSymbol looks way more intuitive while say, debugging with print_family, than VMobjectFromSVGPathString.

Which looks better?

Tex <memory_loc>
        VMobjectFromSVGPathString <memory_loc>
        VMobjectFromSVGPathString <memory_loc>
        VMobjectFromSVGPathString <memory_loc>
        ...
        TexSymbol <memory_loc>
        TexSymbol <memory_loc>
        TexSymbol <memory_loc>

leotrs · 2020-08-31T14:05:18Z

This PR doesn't break code though, it just issues warnings.

I agree TexSymbol looks more convenient.

PgBiel · 2020-08-31T14:05:28Z

Also, TexSymbol looks way more intuitive while say, debugging with print_family, than VMobjectFromSVGPathString.

it sounds like the latter is a more general name. If its only purpose is to serve Tex classes, then maybe it should receive that name, otherwise we could just subclass it with this name, as it could have other uses.

kilacoda-old · 2020-08-31T14:15:13Z

This PR doesn't break code though, it just issues warnings.

I didn't see that earlier. Apologies for that.

leotrs · 2020-08-31T21:27:17Z

This LGTM. Are we waiting for #335 ?

Aathish04 · 2020-09-01T10:12:57Z

Alright, so we can all agree that the DeprecationWarnings are fine for now, right?

About the TexSymbol, I agree that VMobjectFromSVGPathString is way uglier when debugging, but it's only being used in a single place, in the same file, that too.

leotrs · 2020-09-01T10:53:51Z

Alright, so we can all agree that the DeprecationWarnings are fine for now, right?

I count four yeses. (We never decided what actually constitutes a consensus, but having approval from every dev who read this file is a good sign.)

About the TexSymbol, I agree that VMobjectFromSVGPathString is way uglier when debugging, but it's only being used in a single place, in the same file, that too.

Is TexSymbol/VMobjectFromSVGPathString going to ever be used by the end user? If so, I vote we keep it. The latter is more verbose which is useful during development, and the former is more succinct which is convenient for the user. There's nothing wrong in having an alias for convenience I think.

Aathish04 · 2020-09-01T11:16:34Z

About the TexSymbol, I agree that VMobjectFromSVGPathString is way uglier when debugging, but it's only being used in a single place, in the same file, that too.

Is TexSymbol/VMobjectFromSVGPathString going to ever be used by the end user? If so, I vote we keep it. The latter is more verbose which is useful during development, and the former is more succinct which is convenient for the user. There's nothing wrong in having an alias for convenience I think.

I think @kilacoda is in a better position to answer this, since he worked with LaTeX quite a bit when writing Chanim.

As for my opinion, I don't think the end user will ever use TexSymbol since what it does can easily be emulated by SVGMobject and Tex[t]Mobject which are more commonly used and popular anyway.

kilacoda-old · 2020-09-01T11:55:12Z

No, it's not really used by the user, rather it's for splitting up the individual parts of the SVG (path_strings) to be used as indexable submobjects, so it's just a name made to elaborate the fact that it's a part of a TexMobject.

leotrs · 2020-09-01T12:26:54Z

Ok in that case I'm feeling more neutral about it.

But also, it doesn't hurt to have an alias. It doesn't matter if it's used only once in the codebase if it's meant to be used for debugging. In the interest of merging this quickly, let's just leave it and move on.

To clarify: I think leaving it in is the way to go because it provides convenience at very little expense. Removing it may incur in changes to users' or devs' workflows.

Aathish04 · 2020-09-01T16:39:37Z

Alrighty, so the only thing remaining is to port the tests for TextMobject and TexMobject over to their new names, which will be done after #335 is merged. Please review #335 if you haven't yet!

manim/mobject/svg/tex_mobject.py

manim/scene/graph_scene.py

manim/scene/vector_space_scene.py

Co-authored-by: Pg Biel <[email protected]>

Aathish04 · 2020-09-02T03:20:23Z

Unmarking as draft, since the Tex[T]Mobject tests don't need to be ported as they were removed in #335 .

The only test that fails now is the formatting one, which should be fixed in due course I think.

Aathish04 · 2020-09-02T03:34:30Z

I've run the new, updated version of black on the files that have been touched by this PR, as per @leotrs 's suggestion.

Aathish04 added 3 commits August 26, 2020 14:48

Rename TexMobject to MathTex

22562f2

Rename TextMobject to Tex Replace all instances of TexMobject with MathTex and TextMobject with Tex. Get rid of single-use TexSymbol. Rename SingleStringTexMobject to SingleStringMathTex. Add Deprecation warnings to TexMobject and TextMobject.

Merge branch 'master' into mathtex_tex

14a44cc

Edit __all__ for tex_mobject.py

2905ff6

Aathish04 added the enhancement Additions and improvements in general label Aug 31, 2020

Aathish04 added this to the PyPI Release 1.0.0 milestone Aug 31, 2020

Aathish04 requested review from PgBiel, eulertour, huguesdevimeux, kilacoda-old and leotrs August 31, 2020 13:36

Aathish04 self-assigned this Aug 31, 2020

leotrs reviewed Aug 31, 2020

View reviewed changes

Aathish04 added 2 commits September 1, 2020 18:08

Re-add TexSymbol by request of devs.

4f7f545

Merge branch 'master' into mathtex_tex

3be87f7

leotrs reviewed Sep 1, 2020

View reviewed changes

manim/mobject/svg/tex_mobject.py Outdated Show resolved Hide resolved

leotrs approved these changes Sep 1, 2020

View reviewed changes

PgBiel approved these changes Sep 1, 2020

View reviewed changes

Aathish04 and others added 3 commits September 2, 2020 08:22

Merge branch 'master' into mathtex_tex

862eb68

Docstring Modifications by @PgBiel

64a1507

Co-authored-by: Pg Biel <[email protected]>

Import logger differently, by @leotrs

59eda4b

Aathish04 marked this pull request as ready for review September 2, 2020 03:20

Run updated Black on files changed.

75a2739

Aathish04 merged commit 6df53cc into ManimCommunity:master Sep 2, 2020

Aathish04 deleted the mathtex_tex branch September 2, 2020 03:50

naveen521kk mentioned this pull request Apr 19, 2021

Removing TexMobject and TextMobject #1369

Closed

Deprecate TexMobject and TextMobject in favour of MathTex and Tex #391

Deprecate TexMobject and TextMobject in favour of MathTex and Tex #391

Uh oh!

Conversation

Aathish04 commented Aug 31, 2020

List of Changes

Motivation

Explanation for Changes

Testing Status

Further Comments

Acknowledgement

Uh oh!

Uh oh!

leotrs Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

PgBiel Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

leotrs commented Aug 31, 2020

Uh oh!

kilacoda-old commented Aug 31, 2020

Uh oh!

Aathish04 commented Aug 31, 2020

Uh oh!

Aathish04 commented Aug 31, 2020

Uh oh!

leotrs commented Aug 31, 2020

Uh oh!

kilacoda-old commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leotrs commented Aug 31, 2020

Uh oh!

PgBiel commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kilacoda-old commented Aug 31, 2020

Uh oh!

leotrs commented Aug 31, 2020

Uh oh!

Aathish04 commented Sep 1, 2020

Uh oh!

leotrs commented Sep 1, 2020

Uh oh!

Aathish04 commented Sep 1, 2020

Uh oh!

kilacoda-old commented Sep 1, 2020

Uh oh!

leotrs commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Aathish04 commented Sep 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Aathish04 commented Sep 2, 2020

Uh oh!

Aathish04 commented Sep 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Deprecate `TexMobject` and `TextMobject` in favour of `MathTex` and `Tex` #391

Deprecate `TexMobject` and `TextMobject` in favour of `MathTex` and `Tex` #391

kilacoda-old commented Aug 31, 2020 •

edited

Loading

PgBiel commented Aug 31, 2020 •

edited

Loading

leotrs commented Sep 1, 2020 •

edited

Loading