Changing the 'side_length' attribute to a property

[irvanalhaq9]

Overview: What does this pull request change?

This side_length attribute has no effect on the Square class. If I decide to change side_length somewhere else in my code by changing this attribute, nothing happens.

Motivation and Explanation: Why and how do your changes improve the library?

Here is an illustration of modifying the side_length attribute.

sq = Square(side_length=1)
sq.side_length = 3
print(sq.side_length)
print(sq.height)

The side_length does change, but the mobject doesn't change, its height remains the same. But if I change it via the height attribute, it works. However, if I request/change the heigth attribute after a rotation has occurred, the height given is not the side_length.

So I think the code needs to be changed, so that the side_length attribute has an effect and gives consistent results even after a rotation has occurred. After changing it to a property, it works as expected.

Links to added or changed documentation pages

Further Information and Comments

Reviewer Checklist

• [ ] The PR title is descriptive enough for the changelog, and the PR is labeled correctly
• [ ] If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
• [ ] If applicable: newly added functions and classes are tested

[JasonGrace2282]

Do you mind adding a test for this behavior?

[irvanalhaq9]

Hi @JasonGrace2282,

Thank you for your feedback.

I have added tests to cover this behavior. The tests can be found in tests/module/mobject/geometry/test_unit_geometry.py. They specifically verify that:

1. The side_length property reflects the correct value.
2. Changing side_length updates the Square object appropriately.
3. The changes persist correctly even after transformations such as rotations.

Previously, changing the side_length attribute did not affect the Square object as expected. And using the width or height attributes to change the side length of a square after a rotation also gave incorrect results because the height attribute calculates the highest and lowest distances on the y-axis of a mobject, not the side length of the square. By changing side_length to a property, the class now updates correctly and consistently because side_length calculates the distance between two points on its side.

[irvanalhaq9]

Please let me know if there are any additional changes or if further testing is needed.

Thanks!

[irvanalhaq9]

The test seems to fail on Mac. But I think that's because of floating-point on Mac, not because the code breaks other codes. I hope this pull request can be considered for acceptance. Thanks.

[irvanalhaq9]

Hi @JasonGrace2282 ,

I hope you're doing well! I wanted to check in on the status of my pull request #3901. Is there anything else I can do or address to help move it forward?

Thanks for your time!

Just to reiterate, the motivation behind changing side_length to a property was to ensure it has a meaningful impact and utility within the Square class, rather than being a static attribute.

From the currect code:

def __init__(self, side_length: float = 2.0, **kwargs) -> None:
        self.side_length = side_length
        super().__init__(height=side_length, width=side_length, **kwargs)

The parameter side_length, and not self.side_length, is passed directly to the superclass constructor as height and width. Therefore, it seems redundant to store side_length as an instance attribute if it doesn’t have any further impact since it's never utilized elsewhere in the class.

However, if the proposed change to make side_length a property is not deemed necessary, I would like to suggest an alternative approach. Instead of adding it as an attribute, we could directly pass the parameter to the superclass constructor, similar to the approach used by Grant Sanderson in ManimGL:

class Square(Rectangle):
    def __init__(self, side_length: float = 2.0, **kwargs):
        super().__init__(height=side_length, width=side_length, **kwargs)

Thank you.

[JasonGrace2282]

I think the change itself is fine - I've seen enough confusion about this that it's probably better to make it a property.

However, the tests should be working before I'm willing to merge this. Last time I checked, you could get around the floating point error using np.isclose or doing abs(sq.side_length-3)<=1e8 - is there any update on this that I missed?

[irvanalhaq9]

I think the change itself is fine - I've seen enough confusion about this that it's probably better to make it a property.

However, the tests should be working before I'm willing to merge this. Last time I checked, you could get around the floating point error using np.isclose or doing abs(sq.side_length-3)<=1e8 - is there any update on this that I missed?

I apologize for the oversight in the tests.

I’ll be updating the tests shortly to handle the floating-point precision issues as suggested. I'll commit the changes as soon as they are ready. Thanks.

[irvanalhaq9]

Hi @JasonGrace2282,

I wanted to let you know that I’ve addressed the floating-point precision issue based on your suggestions. All the tests are now passing successfully. It seems that I had initially addressed the floating-point precision issue, but I might have made a mistake in my last commit, which led to the problem persisting.

I appreciate your guidance and patience. Please let me know if there are any further adjustments needed.

Thank you!

[irvanalhaq9]

Seems fine to me, thanks.

Thanks for your help and feedback throughout this process.