Skip to content

Fix TypeError resulting from lack of __rmul__#233

Merged
mwcraig merged 1 commit intovpython:masterfrom
UZ9:master
Feb 1, 2024
Merged

Fix TypeError resulting from lack of __rmul__#233
mwcraig merged 1 commit intovpython:masterfrom
UZ9:master

Conversation

@UZ9
Copy link
Contributor

@UZ9 UZ9 commented Sep 26, 2023 •
edited
Loading

The current codebase has this comment regarding the lack of __rmul__ in cyvector.pyx:

    def __mul__(self, other):  ## in cython order of arguments is arbitrary, rmul doesn't exist
        if isinstance(other, (int, float)):
            return vector(self._x * other, self._y * other, self._z * other)
        elif isinstance(self, (int, float)):
            return vector(self * other._x, self * other._y, self * other._z)
        return NotImplemented

However, when cython is not installed the following will throw a TypeError due to a lack of __rmul:

1.0 * vector(3, 4, 8)

This contradicts the following statement in the README:

If you don't have a compilier vpython should still work, but code that generates a lot of vectors may run a little slower.

The commit attached to this PR ensures the __rmul__ seen in vector.py is also present in cypython.pyx.

Relevant discussion on forum:
https://groups.google.com/g/vpython-users/c/6ojA7FxcILA

@UZ9 UZ9 changed the title 🐛 Fix TypeError resulting from lack of __rmul__ Fix TypeError resulting from lack of __rmul__ Sep 26, 2023
@rubdos
Copy link

rubdos commented Nov 29, 2023 •
edited
Loading

(From your comment on the forum)

encounters the TypeError. Seems like there was no rmul for the vector class, with the comment "in cython order of arguments is arbitrary, rmul doesn't exist."

This seems strange considering Cython 3.0.2 is already installed, but something likely worth mentioning in the documentation.

I have Cpython 3.0.5 installed, and I would expect this to "just work" (and be faster) without your patch. Did you find out why having Cython 3.0.x around doesn't help this issue?

@mwcraig
Copy link
Contributor

mwcraig commented Feb 1, 2024

Closing/reopening to run the the tests on Python 3.12

@mwcraig mwcraig closed this Feb 1, 2024
@mwcraig mwcraig reopened this Feb 1, 2024
@mwcraig
Copy link
Contributor

mwcraig commented Feb 1, 2024

Thanks, @UZ9, merging!

@mwcraig mwcraig merged commit b44074c into vpython:master Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@UZ9 @rubdos @mwcraig