ManimCommunity / manim

A community-maintained Python framework for creating mathematical animations.
https://www.manim.community
MIT License
25.16k stars 1.76k forks source link

Some ideas for restructuring Mobject attributes. #787

Open kolibril13 opened 3 years ago

kolibril13 commented 3 years ago

As we are restructuring how manim is configured right now, I have some suggestions:

class Example(Scene):
    def construct(self):
        dot = Dot().set_color(GREEN)
        dot.set_color(RED)
        dot.color=BLUE
        print(dot.color)
        self.add(dot)
        self.wait(1)

This script adds a red dot to the scene, however, it prints the # for blue color. Here it would be nice if dot.color=BLUE would actually change the color to blue on the displayed dot.

Secondly:

class Example(Scene):
    def construct(self):
        dot = Dot(radius=0.1)
        print(dot.radius) # prints 0.1
        dot.set_width(2)
        print(dot.radius) # prints 0.1
        dot.radius = 10
        print(dot.radius) # prints 10
        self.add(dot) # adds dot with radius 2

Here, we can set the width of a dot, which changes its radius, but the attribute parameter of Dot is not touched. At the same time, when the radius attribute of Dot is touched, the displayed dot won't change its shape. I think that this is not a big problem, but in case there is a nice solution to it, that would be great!

And thirdly:

class Example(Scene):
    def construct(self):

        t1 = Text("1")
        t2 = Text("2").next_to(t1)
        t3 = Text("3").next_to(t2)

        self.add(t1,t2,t3)

This is producing White text for all three text Mobjecs. When a white background is used, I think it would be nice to have an easy option to change the default parameter once of Text at the beginning of the script, without the necessity to change it for all three TextMobjects

leotrs commented 3 years ago
  1. Does your code still not work as intended after #751 was merged?

  2. I agree it should be possible to change the general size/shape/geometry of a Mobject by setting some of its attributes. But this also brings up the question of animating methods. For example, it's nice to be able to do self.play(mobject.set_radius, 0.2) to animate the change of size. If we implement a refactor that gets rid of all set_* methods and instead encourages people to do self.radius = 0.2, then this is no longer possible. However, in the interest of getting rid of set_* methods, I think we should implement a single Mobject.set() method that can be passed to play. This was discussed recently with @friedkeenan on discord.

  3. I agree we should have something like config.default_mobject_color that overrides the initial color of all Mobjects.

kolibril13 commented 3 years ago
  1. Does your code still not work as intended after #751 was merged?

I just ran it on the current master and it does not work. I think #751 only fixed that dot.set_color(RED) will change the attribute to red as well, but dot.color=BLUE changes only the attribute, not the color of the displayed dot.

  1. I agree it should be possible to change the general size/shape/geometry of a Mobject by setting some of its attributes. But this also brings up the question of animating methods. For example, it's nice to be able to do self.play(mobject.set_radius, 0.2) to animate the change of size. If we implement a refactor that gets rid of all set_* methods and instead encourages people to do self.radius = 0.2, then this is no longer possible. However, in the interest of getting rid of set_* methods, I think we should implement a single Mobject.set() method that can be passed to play. This was discussed recently with @friedkeenan on discord. Another option would be that both are possible:
    • self.radius = 0.2 in normal code to setup a scene
    • self.play(mobject.set_radius, 0.2) when it is used in an animation code But I also really like the idea of having a Mobject.set() method.
  2. I agree we should have something like config.default_mobject_color that overrides the initial color of all Mobjects.

Indeed, config.default_mobject_color would be good! But I think additionally it would be great if the initial colour could be also set for only one Mobject (like e.g. the Text).

behackl commented 3 years ago

Would it also be possible to work with a static variable here? Like, setting Text.color (or Text.default_color?) to make subsequent calls to Text use the corresponding default color?

leotrs commented 3 years ago

I like that idea a lot! The pro when using the config system is that you can set it once and for all in a user-code config file for example. The con is that it is not as flexible as doing it for each specific class, like you are suggesting.

On Mon, Nov 30, 2020, 4:02 AM Benjamin Hackl notifications@github.com wrote:

Would it also be possible to work with a static variable here? Like, setting Text.color (or Text.default_color?) to make subsequent calls to Text use the corresponding default color?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ManimCommunity/manim/issues/787#issuecomment-735652552, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAILYAHT2CZDBVRQXKXZW3TSSNNSFANCNFSM4UGTFUUQ .

XorUnison commented 3 years ago

As @leotrs has mentioned in response to a question I posted on discord, what about using properties? Using properties should allow us to get the best of all worlds, that is initializing Class(x=y) and changing objects Class.x=y correctly, getting correct values at all times with y=Class.x, while also not having anything like Class.set_x or Class.get_x publicly accessible nor needed. One way to access and set variables that always works. Of course this would mean rewriting tons of code to do this everywhere.

leotrs commented 3 years ago

Yes! I sort of wrote all of the above implicitly assuming that color and similar attributes would be properties 😅

friedkeenan commented 3 years ago

@leotrs and I discussed a potential solution for this on discord a few days ago, so I'll try to condense what we ended up coming up with here. Here though is essentially the conclusion of that conversation on discord.

So basically, all mobject.get_* and mobject.set_* methods would be turned into properties, so get_color and set_color would be turned into a singular color property. This would allow you to do things like obj1.color = obj2.color, and have everything work out as expected, setting obj1's color to obj2's.

The problem with that though is animating setting those properties, because normally you would do something like self.play(obj1.set_color, obj2.get_color()) and that would call the method with the arguments to get two objects to interpolate between. And since you can't do self.play(obj1.color = obj2.color), we need a way to do something similar to that, and which stays mostly consistent with how you'd get and set properties outside of animations.

While we're on the topic of consistency, it doesn't seem especially consistent that when calling a mobject's method outside of an animation, you do it like obj.method(arg), but then when animating it you do self.play(obj.method, arg). They're pretty close, but what if we could get closer? What @leotrs and I came up with at the time was to give every mobject an animate attribute (or anim attribute if animate is deemed too long), which would be of a special class which overrides __getattr__ to do some machinery to allow self.play(obj.animate.method(arg)) instead of self.play(obj.method, arg). The class for the animate attribute could look something like this:

class AnimateUtil:
    def __init__(self, mobject):
        self.mobject = mobject

    def __getattr__(self, attr):
        def real_animate(*args, **kwargs):
            method = getattr(self.mobject, attr)

            return animation_from_method(method, *args, **kwargs)

        return real_animate

This would bring more consistency between how methods are run outside and inside of animations, at least user-facing consistency. This would also greatly improve passing keyword arguments to methods when animating them, since you could do self.play(obj.animate.method(kwarg=blah)) instead of self.play(obj.method, {"kwarg": blah}).

The next part of the puzzle would be a universal set method for mobjects, something like this:

def set(self, **kwargs):
    for attr, value in kwargs.items():
        setattr(self, attr, value)

    return self

Then you could do things like obj.set(color=BLUE), which has the benefit of showing color=BLUE, which is similar to obj.color = BLUE. The main benefit though is that you could then do self.play(obj.animate.set(color=BLUE)), which looks very readable to me, and which also would allow for setting multiple attributes at once in an animation with no extra hassle.

With all of this together, these two would be equivalent ways of changing the color attribute:

obj.color = BLUE
obj.set(color=BLUE)

And these two would be equivalent ways of animating the change of color:

self.play(obj.animate.set(color=BLUE))
self.play(obj.set, {"color": BLUE})

And as @leotrs said on discord, perhaps the fourth one could be deprecated in favor of the third at some point.

Personally, I'm a fan of this approach and I think it gives a really pleasing API for users.

leotrs commented 3 years ago

Just two technical points on the above:

  1. obj.animate.attr(args) returns an instance of Animation. With a little more work, we could allow users to define their own custom methods that return their own custom animations. Note this makes Scene.play's job easier since it no longer has to just stick an ApplyMethod in front of everything.

  2. I am on the fence about deprecating the fourth syntax in the previous comment, perhaps it can still be useful in case some animations are being generated programmatically. But only time will tell!

cobordism commented 3 years ago

Now that #783 has been merged, we can set about restructuring mobject attributes more generally.

I'd be willing to try and implement something along the lines of https://github.com/ManimCommunity/manim/issues/787#issuecomment-737400927 but I can't do it alone. I'll need help.

Am I right in assuming that the 'correct' implementation of these getter/setter methods requires some form of @-decorators?

leotrs commented 3 years ago

Yes! Basically, the following code:

def get_val(self):
    return self.val
def set_val(self, new_val):
    self.val = new_val

is Bad Python, and could/should be replaced with:

@property
def val(self):
    return the_value_of_val
@val.setter
def val(self, val):
    # do something to change the value of val

In the easiest case, the property is just an interface to an underlying attribute.

class MyCircle:
    def __init__(self, radius):
        self.radius = radius
    @property
    def diameter(self):
        return self.radius * 2
    @diameter.setter
    def diameter(self, val):
        self.radius = val / 2

So now you can do both self.diameter = 2 and both the self.radius and self.diameter attributes will reflect that.

This is exactly the intended use-case of properties. I think that a good first pass is to try to deprecate getters and setters and replace them with properties all over the codebase, as much as possible.

leotrs commented 3 years ago

Perhaps @eulertour or @PgBiel want to chime in. This is the moment to retake our discussion about dataclasess and attrs, perhaps?

kolibril13 commented 3 years ago

One furhter idea I would find very useful: When this script

        s1 = Square().set_color(BLUE)
            s2 = Square().set_color(BLUE)
            s3 = Square().set_color(BLUE)
            s4 = Square().set_color(BLUE)
            s5 = Square().set_color(BLUE)
            s6 = Square().set_color(RED)
            s7 = Square().set_color(RED)
            s8 = Square().set_color(RED)

would be equal to this script

            Square().set_defaut_color(BLUE)
            s1 = Square()
            s2 = Square()
            s3 = Square()
            s4 = Square()
            s5 = Square()
            Square().set_defaut_color(RED)
            s6 = Square()
            s7 = Square()
            s8 = Square()
friedkeenan commented 3 years ago

Could probably just get away with adding a default_color attribute to the Mobject class, and then do Square.default_color = BLUE, which will change the default_color attribute for Square and its descendants (unless those descendants changed the attribute already), but not for other classes.

cobordism commented 3 years ago

It would be nice to have the attribute listed as a keyword argument, because that helps surface the option in the IDE.

If we have the default values as class variables, and attribute=None in the init function signature, the body needs to have: if attribute is None: attribute =Class.default_for_attribute self.attribute = attribute.

... it could work. Pythonistas, is that an acceptable pattern?

(is there a variable like self, but for the class instead of the instance?)

friedkeenan commented 3 years ago

Feels weird to me that you'd set a default for a class in an instance's init function.

(is there a variable like self, but for the class instead of the instance?)

You can get the class with type(self). Alternatively you could set it with a classmethod, and in classmethods the first argument is the class, like so:

class Test:
    @classmethod
    def test(cls):
        print(cls.__name__)

class Test2(Test):
    pass

Test.test()  # Prints "Test"
Test2.test() # Prints "Test2"
behackl commented 3 years ago

It would be nice to have the attribute listed as a keyword argument, because that helps surface the option in the IDE.

Everything that can be changed for an instance of an object should appear in the constructor, true. But I feel it would be very clumsy to be able to set the default class behavior from an instance, just as @friedkeenan also has mentioned.

Class attributes can (and should!) be documented as well. I am not entirely sure what you mean when you say that it helps to surface the options in the IDE (from tab-completion?) -- but I know that class attributes can appear in the documentation and that they can be found in the IDE when typing something like Square., or even Square.<tab> from the interpreter.

cobordism commented 3 years ago

nobody said anything about setting class defaults from an instance. :confused:

friedkeenan commented 3 years ago

It would be nice to have the attribute listed as a keyword argument

I figured you meant putting e.g. default_color=None in the init function signature, but upon reread it seems that you meant putting color=None in the init function signature?

leotrs commented 3 years ago

Could probably just get away with adding a default_color attribute to the Mobject class, and then do Square.default_color = BLUE, which will change the default_color attribute for Square and its descendants (unless those descendants changed the attribute already), but not for other classes.

I think this is the way to go here. (note @kolibril13 there's no need for get or set methods).

@cobordism I don't think that it is possible to use a class attribute as a default value in a method signature. (The reason is that the method needs to be defined before the class is defined, but the class needs to be defined before the class attribute is defined. So using the latter to define the former is impossible.) So the IDE will only ever be able to show a constant (which may be outdated), or None (which may be uninformative). I don't see how the IDE can show the actual value here so I think it's best to just use None, and replace it with the class attribute within the body of __init__ as you suggested.

is there a variable like self, but for the class instead of the instance?

Yes, in meta classes.

cobordism commented 3 years ago

I agree with this too.

I'd like to attempt one more clarification. I had suggested adding attribute=None to the init arguments and then setting it to the class default in the body (as indicated in previous comment) as opposed to not explicitly having it listed as a keyword argument and instead setting it via attribute = kwargs.get("attribute", class_var_default). This is functionally equivalent, but the IDE does not show attribute as a possible keyword argument. That's all I meant. Apologies for being unclear. I'm on the road, typing from phone. I may have been too curt.

The nice thing about the whole Square.default_colour suggestion is that it allows very easily changing defaults by making subclasses just as had been possible with the CONFIG dicts, but without the mess of digest_config().

XorUnison commented 3 years ago

@eulertour This is the other issue on implementing such properties in manim I was talking about btw, so if we bring this up in the next meeting you should be up to date with this thread.

friedkeenan commented 3 years ago

I took another look at the set_* and get_* methods there currently are, and I noticed that some setters take extra arguments, for instance set_height and set_width have a stretch argument, and things like set_color and set_opacity have arguments to tell them whether or not to set things recursively. This poses a problem since properties can only take one argument when being set: the value they're being set to. So I've been mulling this over and I've come up with a few solutions that work but none that I'm particularly happy with.

So first, we could continue support for the setters we have now but make sure they're able to accept just the value to set with no other arguments so we can use them with properties, something like this:

class MyMobject:
    def get_width(self):
        return 1

    def set_width(self, value, stretch=False):
        # Set the width based on `value` and `stretch`
        pass

    width = property(get_width, set_width)
    # Or perhaps we could do some fun stuff
    # with Mobject's __getattr__ method to
    # automatically return the correct functions
    # for `set_width` and `get_width` by just defining
    # the `width` property.

m = MyMobject()

# These both return the width
m.width
m.get_width()

# These both set the width without stretching
m.width = 1
m.set_width(1)

# This sets the width with stretching
m.set_width(1, stretch=True)

I'm not too fond of this approach because it means there are some cases where you need to use the old setters instead of properties, which poses consistency issues and doesn't feel very pythonic. It would also present yet another way to set attributes if we add the general set() method.

The second way I was thinking of (which is really two ways in the same vein of thought) is that either we decide that those extra arguments to setters don't matter at all and just remove them altogether, which would pose problems for e.g. Ellipse which needs the stretch argument for setting width/height, or we could add properties for each variation of arguments to the setter, like so:

class MyMobject:
    @property
    def width(self):
        return 1

    @width.setter
    def width(self, value):
        # Set the width without stretching
        pass

    @width.setter
    def width_stretch(self, value):
        # Set the width with stretching
        pass

m = MyMobject()

# Both return the width
m.width
m.width_stretch # If we want this could be changed to be set-only

# Sets the width without stretching
m.width = 1

# Sets the width with stretching
m.width_stretch = 1

I might like this approach more than the first, but I also find it inelegant and wordy and not very scalable if the setter needs a fair amount of variations.

The third way I thought of is to create our own property that forwards a dictionary of keyword arguments onto the setter, like so:

class kwproperty(property):
    @staticmethod
    def has_kwargs(value):
        # A value has kwargs if it's a tuple of length 2
        # and the second element is a dictionary.

        return isinstance(value, tuple) and len(value) == 2 and isinstance(value[1], dict)

    def __set__(self, instance, value):
        if self.has_kwargs(value):
            value, kwargs = value

            self.fset(instance, value, **kwargs)
        else:
            super().__set__(instance, value)

class MyMobject:
    @kwproperty
    def width(self):
        return 1

    @width.setter
    def width(self, value, stretch=False):
        # Set the width based on `value` and `stretch`
        pass

m = MyMobject()

# Returns the width
m.width

# Sets the width without stretching
m.width = 1

# These both set the width with stretching
m.width = 1, {"stretch": True}
m.width = (1, {"stretch": True})

I'm really not sure how I feel about this approach. The biggest downside probably is that it's very similar to how keyword arguments used to get passed to method animations before the .animate syntax was added. It would also look weird in general set() method calls, because it'd have to look like m.set(width=(1, {"stretch": True})), but perhaps if needing to pass extra set args is rare enough, then it could be acceptable? Really not sure.

The last approach I thought of is one where we create our own property and manage the extra set args using with blocks, like so:

import contextlib

class ctxproperty(property):
    def __set_name__(self, owner, name):
        self.name = name

    def __set__(self, instance, value):
        extra_set_args = instance._extra_set_args.get(self.name, {})
        self.fset(instance, value, **extra_set_args)

class MyMobject:
    def __init__(self):
        self._extra_set_args = {}

    @contextlib.contextmanager
    def extra_set_args(self, attr, **kwargs):
        # Perhaps this could be named `set_options` instead?

        try:
            self._extra_set_args[attr] = kwargs
            yield
        finally:
            self._extra_set_args.pop(attr)

    @ctxproperty
    def width(self):
        return 1

    @width.setter
    def width(self, value, stretch=False):
        # Set the width based on `value` and `stretch`
        pass

m = MyMobject()

# Returns the width
m.width

# Sets the width without stretching
m.width = 1

# Sets the width with stretching
with m.extra_set_args("width", stretch=True):
    m.width = 1

# Once again sets the width without stretching
m.width = 1

This approach is probably the one I like the most, but not by a terribly big margin. I like how it's more explicit and readable, and how you set the extra arguments like how you would pass them to a function, not as a dictionary like the previous suggestion, and I like how it's more scalable unlike the second suggestion, and I like how it doesn't require the use of a setter function when setting the attribute, unlike the first suggestion. It also meshes well with the general set() method, unlike the previous suggestion. What I don't like is how the extra arguments could in theory be far away from where the attribute is actually set, which could decrease readability, and in general it's another thing to keep track of when reading code, whereas the other solutions all have the extra arguments where the attribute is being set. I'm also not thrilled that you'd pass the attribute name as a string in the with statement, but I don't think that can be helped and it's not that big of a deal.

Aside from all this though, it might be best to create our own subclass of property anyways when it comes to default attributes for classes, so that if an attribute hasn't been set yet we can have it return the default for the class, however that gets set. Though I guess that would get weird if you changed the default color of a class and then previously defined mobjects change color.

But if we do go and create our own property, we could be cutesy and call it mattribute which I think would be a big plus.

leotrs commented 3 years ago

Out of all the options, I think the first one is the most pythonic. I have to say I don't think the others are very viable.

Another options is for Ellipse to have a new property called stretch. Then, you can do either

ellipse.stretch = True
ellipse.width = 10

or just

ellipse.width = 10

And then remember to set ellipse.stretch every time you need it (or just set it once at the start and leave it alone). I think this is more pythonic than the above, and it's very close to the fourth proposal that uses context managers.

Before deciding on these, though, it would be interesting to know how many setter methods accept more than one arguments. If it's just a few, then they can be handled on a case-by-case basis, and there is no need to think about implementing context managers or custom properties. If there are a ton of them (or we expect to have many in the future), then we can talk about more involved solutions.

friedkeenan commented 3 years ago

These are the setters I found from a grep of def set_ that I think would be reasonable to change into properties and which accept arguments beyond the value to set, excluding setters that override parent setters:

Decimal.set_value Mobject.set_width Mobject.set_height Mobject.set_depth Mobject.set_x Mobject.set_y Mobject.set_z Mobject.set_color VMobject.set_fill VMobject.set_stroke VMobject.set_opacity VMobject.set_sheen_direction VMobject.set_sheen VMobject.set_shade_in_3d

It's not that many but they also cover attributes that I think are used fairly often. For some, like set_stroke, it might be reasonable to split it up into stroke_color, stroke_width, etc. properties and keep set_stroke but make it set those properties according to the args, and then maybe for the attributes which have arguments for recursing into submobjects, like set_color, we could break it up into color and color_norecurse properties or something, or maybe we could decide that they should always be recursive. But that still leaves things like set_width that perhaps can't be handled so nicely.

To be clear, I'm all for handling these things on a case by case basis, but it would also be very nice to have a framework in place for places where it can't be neatly handled in addition to dealing with each case on its own merits.

My main problem with my first suggestion is that it feels like adding more complexity without simplifying things or truly fixing the issue we currently have with getters/setters. There would be some situations where you can use the nice, pythonic properties, but then other situations where you need to use the less pythonic setters. My ideal for a solution to our current situation would be for properties to supersede getters/setters in every situation and for the latter to get deprecated or removed. I will admit that the first suggestion is the simplest/least "hacky" to implement and perhaps users would understand it better, but when it comes to actual use, I'm just simply not a fan of the inconsistency it necessitates, and it's in all honesty probably not that big of a deal, but I'd really like the new system to be the best we can make it, not just good enough.

As for setting the stretch attribute to True, then setting the width/height, and then setting stretch back to False, I'm less a fan of that than my first suggestion. I don't like that it's not necessarily connected to the width/height, that it's not necessarily visually apparent that the mobject is in a certain temporary state, and I don't like how you need to remember to set it back to what it was before, whatever it was before, because it might not even be False necessarily.

In the end though, I'd be fine with the first suggestion, I just wanted to throw ideas out to see what stuck and what other thoughts people might have, because I'd like to start work on migrating to the new system (probably in incremental PRs while keeping backwards compatibility, at least until everything's migrated) but came upon this hitch. And I may have gotten a little carried away with zany python machinery trying to come up with a solution.

Aside from all this though, it might be best to create our own subclass of property anyways when it comes to default attributes for classes, so that if an attribute hasn't been set yet we can have it return the default for the class, however that gets set. Though I guess that would get weird if you changed the default color of a class and then previously defined mobjects change color.

Touching again on this though, perhaps if our theoretical custom property were set to None, then it would set itself to the class default if available, which would avoid lots of self.attr = attr or self.default_attr lines, and would allow users to do obj.attr = None to get back to the default at any time. And if we were to go this route, I'd like to do it in the same stroke as migrating to properties in general, if only to avoid a PR changing the use of property to our custom property (which from this point forth I will be referring to as mattribute because that name is too good to not use at all).

friedkeenan commented 3 years ago

Touching again on this though, perhaps if our theoretical custom property were set to None, then it would set itself to the class default if available, which would avoid lots of self.attr = attr or self.default_attr lines, and would allow users to do obj.attr = None to get back to the default at any time. And if we were to go this route, I'd like to do it in the same stroke as migrating to properties in general, if only to avoid a PR changing the use of property to our custom property (which from this point forth I will be referring to as mattribute because that name is too good to not use at all).

To further expand on this, here's a quick proof of concept showing what I mean:

class mattribute(property):
    def __set_name__(self, owner, name):
        self.default_name = "default_" + name

    def __set__(self, instance, value):
        if value is None:
            # Should it error if there's no default set and None is passed?
            value = getattr(instance, self.default_name, None)

        super().__set__(instance, value)

class MyMobject:
    default_width = 1

    def __init__(self, width=None):
        self.width = width

    @mattribute
    def width(self):
        return self._width

    @width.setter
    def width(self, value):
        self._width = value

m = MyMobject()

# Returns 1
m.width

m.width = 2

# Returns 2
m.width

# Sets it back to the default
m.width = None

# Returns 1
m.width

m = MyMobject(2)

# Returns 2
m.width

To further further add onto this, you could do something like this to speed up defining simple mattributes:

class mattribute(property):
    def __init__(self, fget=None, fset=None, fdel=None, doc=None):
        if isinstance(fget, str):
            doc = fget
            fget = None

        if fget is None and fset is None and fdel is None:
            fget = lambda x:    getattr(x, self.private_name)
            fset = lambda x, v: setattr(x, self.private_name, v)
            fdel = lambda x:    delattr(x, self.private_name)

        super().__init__(fget, fset, fdel, doc)

    def __set_name__(self, owner, name):
        self.default_name = "default_" + name
        self.private_name = "_" + name

    def __set__(self, instance, value):
        if value is None:
            # Should it error if there's no default set and None is passed?
            value = getattr(instance, self.default_name, None)

        super().__set__(instance, value)

class MyMobject:
    default_width = 1

    def __init__(self, width=None):
        self.width = width

    width = mattribute("Docstring for width attribute")

So that simple attributes can still easily take advantage of the automatic defaulting

leotrs commented 3 years ago

I think we are taking an interpretation of "pythonic" that is too literal. In the end, properties are meant to replace the following pattern:

obj.get_x()
obj.set_x(10)

for the following:

obj.x
obj.x = 10

However, in some of the above mentioned cases, we do not have this pattern. What we have instead is obj.set_x(x, arg1, arg2, ...). In this case, it doesn't make much sense to me to try to force the syntax in order to use properties.

I still think the way to go is your first suggestion, and encourage as much as possible the use of properties. If the underlying member requires other arguments, then it also gets a setter method.

This also does not increase any complexity at all, since the property can just call the method with some default values:

class MyClass:
    @property
    def x(self):
        return self._x

    @x.setter
    def x(self, value):
        self.set_x(value, arg1=some_def_value)

    def set_x(self, value, arg1):
        ### some logic here

This does not increase any complexity in the dev side as all the logic is executed in a single place, though it does overload the user interface a bit.

I do not mean to sound harsh, but I think that most/all other suggestions in this thread are over-engineering :upside_down_face:

friedkeenan commented 3 years ago

I'm fine with my first suggestion, I'm just not overjoyed about it, you know? But yeah, it probably is the best way out of the four suggestions I gave.

This does not increase any complexity in the dev side as all the logic is executed in a single place, though it does overload the user interface a bit.

This is what I meant by complexity. Users would have to know two different APIs and when to use one over the other, and while I don't think that's ideal necessarily, it is again probably the best option.

I do not mean to sound harsh, but I think that most/all other suggestions in this thread are over-engineering :upside_down_face:

This is probably also true. I really like the control Python gives you over how things work, and I really like the machinery that comes out of that control, so sometimes I do get a little lost in making that machinery, and I probably did do that with my earlier comment with the four suggestions. However, I do think my idea about the automatic defaulting is a good one and would ultimately make the code simpler and easier to extend and just generally better, even if understanding something takes the initial hurdle of understanding what mattribute does (which would be aided by documentation and comments). You could get a similar interface with normal properties and calling a function in the setter to transform the value into the default when necessary, but I think that would lead to a lot of unnecessary duplicate code. You could forgo that "setting to None sets it to the default" system entirely, but again I think it would lead to worse code. There would be lots of lines of self.attr = attr or self.default_attr, but then even that won't work all the time, because take for instance if the attribute is an int, something that can be implicitly converted to a bool:

def default(x=None):
    return x or 1729

# Returns 1729
default()

# Returns 1
default(1)

# Returns 1729
default(0)

So then, at least in some cases, you'd want to specify if attr is None:, which leads to greater wordiness, but then also if we wanted to say, deepcopy the defaults to avoid mutable weirdness similar to having mutable types as default arguments in a function, then it'd be even wordier.

So ultimately, I do really think sectioning off/abstracting the defaulting code would be better than otherwise, though perhaps defaulting stuff would be outside the scope of the initial transition from attributes to properties?

leotrs commented 3 years ago

So ultimately, I do really think sectioning off/abstracting the defaulting code would be better than otherwise, though perhaps defaulting stuff would be outside the scope of the initial transition from attributes to properties?

Yes please let's leave this to a future conversation.

friedkeenan commented 3 years ago

Just to update, now that #995 has been merged, incremental PRs migrating specific mobject attributes to properties can be made