Attrs - Initial progress #271

azarzadavila · 2020-08-12T17:36:37Z

As discussed in #7 @PgBiel and @kilacoda started working on progressively ditching CONFIG to use instead attrs.
By conversing with PgBiel I thought I could give a bit of help. This PR is currently more a way to see and discuss how attrs could look like in the project.
I would also like to bring a few issues with using this approach.

List of Changes

Mobject using attrs
geometry.py using attrs
test_geometry.py compatible with the changes

Issues with attrs

attrs is not really inheritance friendly. As manim abundantly uses inheritance this results in a few issues/shortcomings.

MAIN ISSUE A class that inherits from a class that possesses a keyword argument for initialization can not have a positional argument.
This means for example that if TipableVMobject uses attrs and has a keyword argument than Line can not use start and end as positional arguments.
One advantage of using attrs is to specify a type to each attribute. However if one wants to redefine the default value for a parent attribute, than the type of said attribute must be re-specified. For example, Circle that uses RED as default subclasses Arc that uses WHITE as default. Both classes must specify the type of arg color. It could look like this :

class Arc(TipableVMobject):
    color: typing.Union[str, Color] = WHITE

class Circle(Arc):
    color: typing.Union[str, Color] = RED

Thus repeating typing.Union[str, Color].

Possible solutions

attrs suggests the use of @classmethod for some initialization.
This could be used for cases where the arguments in the master __init__ version uses positional arguments and/or the passed arguments should not be attributes of the class. For example a method for Line could be included :

    @classmethod
    def from_start_end(cls, start, end, **kwargs):
        return cls(start=start, end=end, **kwargs)

a special validator could be used to indicate some initialization arguments can not be None.
This would still require the use of keyword arguments but it would ensure that they are passed anyway.

These solutions would still break code that includes positional arguments.

Testing Status

It passes two tests :

test_creation.py
test_geometry.py which has been modified to remove positional arguments.

TODO

find a way to still keep CONFIG for backward compatibility reasons. This should not be too hard, I believe it is just a matter of placing an adequate function in __attrs_post_init__.
choose adequate types for all attributes
implement adequate validators
further replace CONFIG in other parts of the code

Further Comments

A custom decorator dclass was generated to not repeat what was chosen as parameters in the attrs decorator :

dclass = functools.partial(attr.s, auto_attribs=True, eq=False)

(slots=True will normally also be part of final implementation).
Custom decorators like this are not analyzed by PyCharm this means the attributes are not part of the autocompletion.
However attr.s is recognized by PyCharm (I don't know for other IDEs) so I think I'll prefer to rewrite each time the decorator.

I hope this will bring more insight on how attrs could be integrated to manim and I'll try to keep working on this PR this week.

Acknowledgement

I have read the Contributing Guidelines

…ssed along to NumberLine

PgBiel

there's just way too much for me to review right now... also the master merge is kinda annoying while reviewing ngl but i'll let it be for now

apply my comments so far to all files. In general, the float attributes are being set to ints: either set them to .0 or Union[int, float]. Also, unnecessary tp.Anys are being used. Finally, we have to specify the types of elements in lists!

PgBiel · 2020-08-16T15:31:11Z

manim/animation/composition.py

+    # If None, this defaults to the sum of all
+    # internal animations
+    run_time: float = None
+    rate_func: tp.Any = linear


???

Suggested change

rate_func: tp.Any = linear

rate_func: typing.Union[typing.Callable[[float, float], float], typing.Callable[[float], float]] \

= linear

PgBiel · 2020-08-16T15:32:06Z

manim/animation/composition.py

+    # If >0 and <1, they start at lagged times
+    # from one and other.
+    lag_ratio: float = 0
+    group: tp.Any = None


???
(Note: normally this would be Optional[Group], but since on init it is always set to a Group, then it's ok to have it as None)

Suggested change

group: tp.Any = None

group: Group = None

PgBiel · 2020-08-16T15:32:35Z

manim/animation/composition.py

+    # from one and other.
+    lag_ratio: float = 0
+    group: tp.Any = None
+    animations: tp.List = attr.ib(default=attr.Factory(list))


list of what? also just use this

Suggested change

animations: tp.List = attr.ib(default=attr.Factory(list))

animations: tp.List[Animation] = attr.ib(factory=list)

PgBiel · 2020-08-16T15:34:40Z

manim/animation/creation.py

 class Uncreate(ShowCreation):
-    CONFIG = {"rate_func": lambda t: smooth(1 - t), "remover": True}
+    rate_func: tp.Any = lambda t: smooth(1 - t)


Suggested change

rate_func: tp.Any = lambda t: smooth(1 - t)

rate_func: typing.Union[typing.Callable[[float, float], float], typing.Callable[[float], float]] \

= lambda t: smooth(1 - t)

PgBiel · 2020-08-16T15:35:13Z

manim/animation/indication.py

-        "scale_factor": 1.2,
-        "color": YELLOW,
-    }
+    rate_func: tp.Any = there_and_back


Suggested change

rate_func: tp.Any = there_and_back

rate_func: typing.Union[typing.Callable[[float, float], float], typing.Callable[[float], float]] \

= there_and_back

PgBiel · 2020-08-16T15:50:48Z

manim/mobject/geometry.py

-        "color": WHITE,
-    }
+    radius: float = DEFAULT_DOT_RADIUS
+    stroke_width: float = 0


that or change to 0.0

Suggested change

stroke_width: float = 0

stroke_width: tp.Union[int, float] = 0

Suggested change

stroke_width: float = 0

stroke_width: float = 0.0

PgBiel · 2020-08-16T15:51:22Z

manim/mobject/geometry.py

+    inner_radius: float = 1.0
+    outer_radius: float = 2.0
+    angle: float = TAU / 4
+    start_angle: float = 0


that or change to 0.0

Suggested change

start_angle: float = 0

start_angle: tp.Union[int, float] = 0

Suggested change

start_angle: float = 0

start_angle: float = 0.0

PgBiel · 2020-08-16T15:52:03Z

manim/mobject/geometry.py

-        "buff": 0,
-        "path_arc": None,  # angle of arc specified here
-    }
+    buff: float = 0


Suggested change

buff: float = 0

buff: tp.Union[int, float] = 0

Suggested change

buff: float = 0

buff: float = 0.0

PgBiel · 2020-08-16T15:52:35Z

manim/mobject/geometry.py

-        "angle": 0,
-    }
+    width: float = 0.2
+    angle: float = 0


Suggested change

angle: float = 0

angle: tp.Union[int, float] = 0

Suggested change

angle: float = 0

angle: float = 0.0

PgBiel · 2020-08-16T15:53:04Z

manim/mobject/geometry.py

+    stroke_width: float = 6.0
+    buff: float = MED_SMALL_BUFF
+    max_tip_length_to_length_ratio: float = 0.25
+    max_stroke_width_to_length_ratio: float = 5


Suggested change

max_stroke_width_to_length_ratio: float = 5

max_stroke_width_to_length_ratio: tp.Union[int, float] = 5

Suggested change

max_stroke_width_to_length_ratio: float = 5

max_stroke_width_to_length_ratio: float = 5.0

leotrs · 2020-08-15T19:37:06Z

manim/mobject/coordinate_systems.py

-    def __init__(self, **kwargs):
-        VGroup.__init__(self, **kwargs)
+    def __attrs_post_init__(self):
+        VGroup.__attrs_post_init__(self)


Question: why not use super()?

leotrs · 2020-08-15T19:37:53Z

manim/mobject/coordinate_systems.py

        self.x_axis = self.create_axis(self.x_min, self.x_max, self.x_axis_config)
        self.y_axis = self.create_axis(self.y_min, self.y_max, self.y_axis_config)
        self.y_axis.rotate(90 * DEGREES, about_point=ORIGIN)
        # Add as a separate group incase various other
        # mobjects are added to self, as for example in
        # NumberPlane below
-        self.axes = VGroup(self.x_axis, self.y_axis)
+        self.axes = VGroup.from_vmobjects(self.x_axis, self.y_axis)


If VGroup.add is made to perform input checking, then VGroup.from_vmobjects() can become VGroup(), yes?

leotrs · 2020-08-15T19:39:25Z

manim/mobject/geometry.py

 class CurvedArrow(ArcBetweenPoints):
-    def __init__(self, start_point, end_point, **kwargs):
-        ArcBetweenPoints.__init__(self, start_point, end_point, **kwargs)


I think super() can be weird when multiple inheritance is involved. In other cases, it's fine.

leotrs · 2020-08-16T17:27:44Z

This PR is way too large to review effectively. In the future we should make smaller, incremental, self-contained PRs in general

PgBiel · 2020-08-16T17:31:53Z

In the future we should make smaller, incremental, self-contained PRs in general

In fact, I think we should start immediately. Our plan was to split our work in attrs; I proposed doing one PR per folder, while EulerTour proposed a further division. We can't work with a PR for everything. So I propose we split this here and now

leotrs · 2020-08-16T17:34:03Z

Yep, let's do it. 251 files in a single PR is unmanageable I think.

azarzadavila · 2020-08-16T19:46:50Z

Okey, sorry I was not sure how to proceed with this PR. I also thought merging master would look nicer than just show all commits since beginning of the PR. I wanted to be able to pass all tests before putting this ready to review. How were you thinking of proceeding? Perhaps it would be interesting to keep a separate branch and applying multiple PRs to that branch until it has at least everything to pass the tests. For example, I think it was necessary once Mobject pass to attrs every Mobject subclass should also use attrs to avoid any errors.

leotrs · 2020-08-16T21:34:32Z

Whoever is working on the attrs branch should periodically merge master into attrs (NOT the other way around). And attrs should
be merged into master frequently once each file or folder is done.

leotrs · 2020-08-16T21:35:06Z

I think the way forward here is to delete this PR, merge master into attrs and resolve merge conflicts, and PR from attrs again, only a few files at a time if possible.

PgBiel · 2020-08-16T21:58:51Z

Okey, sorry I was not sure how to proceed with this PR.

It's perfectly fine! Sorry if it sounded like some sort of rant or something lol, I should have expressed myself better... Just, it needed to be mentioned

I also thought merging master would look nicer than just show all commits since beginning of the PR.

We can prob do this in the attrs branch as leo proposed above

I wanted to be able to pass all tests before putting this ready to review.

Well, that's certainly an issue: by splitting PRs, tests won't pass just yet. But once we build the attrs branch slowly (see below), at the end, we will be able to verify tests with the "big PR".

How were you thinking of proceeding?

(see below)

Perhaps it would be interesting to keep a separate branch and applying multiple PRs to that branch until it has at least everything to pass the tests.

The idea is similar: we'd go merging PRs until we finish the job, then we'd make one master PR (haha get it) (ok sorry that was bad) to concretize the changes

For example, I think it was necessary once Mobject pass to attrs every Mobject subclass should also use attrs to avoid any errors.

Doing this would generate necessarily big PRs. We can't depend on the inheritance tree. There are better options, such as per folder or idk 2 per folder, gotta see that

PgBiel · 2020-08-16T22:00:09Z

I think the way forward here is to delete this PR, merge master into attrs and resolve merge conflicts, and PR from attrs again, only a few files at a time if possible.

sure thing bud

leotrs · 2020-08-23T19:48:33Z

@azarzadavila @PgBiel

As per the comments here, it doesn't seem like we are going to move forward with this PR, right? It is my understanding that this will be broken down into several smaller PRs. If that's the case, please close this one, else please update on the status.

leotrs · 2020-10-08T21:37:08Z

I'm closing this for lack of activity.

azarzadavila and others added 30 commits July 24, 2020 19:31

Recognize verbose in config

87fd921

Pass verbose to ffmpeg

07512aa

Separate verbosity level of project with ffmpeg loglevel

48827d5

!fiuxp remove SILENT in loglevel

707395b

Show ouput of open_file_if_needed only on DEBUG

4f39036

Quiet equivalent to CRITICAL level

3635ec7

Disable status bar if verbose level not DEBUG/INFO

a75611d

Remove comment set logger level

b2bdbe9

Add option progress_bar on/off

eee9874

Change ffmpeg loglevel to a string

624b6ab

Change ffmpeg option to ffmpeg_loglevel

4a49e4a

VERBOSE_CHOICES through dict keys from FFMPEG_MAP

794c8e0

Remove quiet flag

1988237

Remove disable var for progress_bar

6f208e4

ffmpeg_loglevel only in config file

520d1c0

progress_bar flag to boolean

ce7cecb

progress_bar independant on verbose flag

6209cdf

Comments on verbose/ffmpeg loglevel/progress_bar flags

0104d6e

Remove metavar to verbose

67c12fc

Merge remote-tracking branch 'fork/master' into verbose

7c1c11b

Black config_utils.py

097998a

Merge remote-tracking branch 'fork/master' into verbose

9498cfa

initial sphinx setup

94f004d

add build dir to gitignore

f5cadb1

add theme req

f2e375d

Add x_axis_config and y_axis_config CONFIG options

72197a2

Add test for NumberLine config

7a18c5a

Remove CONFIG entries that are not used by GraphScene and are only pa…

3f5002e

…ssed along to NumberLine

Fix formatting

9d986c8

Fix formatting

bb3bed0

azarzadavila added 16 commits August 15, 2020 22:07

Transform with attrs

9ab6d8f

FadeInFrom, FadeInFromDown with attrs

d4a6fde

ShowPartial, ShowCreation, Uncreat with attrs

690bd28

Indicate and CircleIndicate with attrs

176322d

AnimationGroup with attrs

eba7bb5

Flash with attrs

d2beea3

ShowPassingFlash, ShowCreationThenDestruction with attrs

cb21384

Succession, ShowCreationThenFadeOut with attrs

e18323a

AnimationOnSurroundingRectangle with attrs

a81315f

ShowPassingFlashAround with attrs

8b3367a

MoveToTarget with attrs

ffd6c6f

ClockwiseTransform with attrs

6b9943f

ReplacementTransform, TransformFromCopy with attrs

ccb0440

Transform with target_mobject keyword in tests

c1e2233

Check for add in VGroup

0d86653

Merge branch 'master' into attrs

58472f6

PgBiel requested changes Aug 16, 2020

View reviewed changes

leotrs reviewed Aug 16, 2020

View reviewed changes

eulertour mentioned this pull request Oct 4, 2020

CONFIG dictionaries stuff #7

Closed

leotrs closed this Oct 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attrs - Initial progress #271

Attrs - Initial progress #271

azarzadavila commented Aug 12, 2020

PgBiel left a comment

PgBiel Aug 16, 2020

PgBiel Aug 16, 2020

PgBiel Aug 16, 2020

PgBiel Aug 16, 2020

PgBiel Aug 16, 2020

PgBiel Aug 16, 2020

PgBiel Aug 16, 2020

PgBiel Aug 16, 2020

PgBiel Aug 16, 2020

PgBiel Aug 16, 2020

leotrs Aug 15, 2020

leotrs Aug 15, 2020

leotrs Aug 15, 2020

leotrs commented Aug 16, 2020

PgBiel commented Aug 16, 2020

leotrs commented Aug 16, 2020

azarzadavila commented Aug 16, 2020

leotrs commented Aug 16, 2020

leotrs commented Aug 16, 2020

PgBiel commented Aug 16, 2020 •

edited

Loading

PgBiel commented Aug 16, 2020

leotrs commented Aug 23, 2020

leotrs commented Oct 8, 2020

	rate_func: tp.Any = linear
	rate_func: typing.Union[typing.Callable[[float, float], float], typing.Callable[[float], float]] \
	= linear

	animations: tp.List = attr.ib(default=attr.Factory(list))
	animations: tp.List[Animation] = attr.ib(factory=list)

	rate_func: tp.Any = lambda t: smooth(1 - t)
	rate_func: typing.Union[typing.Callable[[float, float], float], typing.Callable[[float], float]] \
	= lambda t: smooth(1 - t)

	rate_func: tp.Any = there_and_back
	rate_func: typing.Union[typing.Callable[[float, float], float], typing.Callable[[float], float]] \
	= there_and_back

	stroke_width: float = 0
	stroke_width: tp.Union[int, float] = 0

	max_stroke_width_to_length_ratio: float = 5
	max_stroke_width_to_length_ratio: tp.Union[int, float] = 5

Attrs - Initial progress #271

Attrs - Initial progress #271

Conversation

azarzadavila commented Aug 12, 2020

List of Changes

Issues with attrs

Possible solutions

Testing Status

TODO

Further Comments

Acknowledgement

PgBiel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leotrs commented Aug 16, 2020

PgBiel commented Aug 16, 2020

leotrs commented Aug 16, 2020

azarzadavila commented Aug 16, 2020

leotrs commented Aug 16, 2020

leotrs commented Aug 16, 2020

PgBiel commented Aug 16, 2020 • edited Loading

PgBiel commented Aug 16, 2020

leotrs commented Aug 23, 2020

leotrs commented Oct 8, 2020

PgBiel commented Aug 16, 2020 •

edited

Loading