Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vector multiplications #1347

Merged
merged 7 commits into from
Jun 7, 2024
Merged

Vector multiplications #1347

merged 7 commits into from
Jun 7, 2024

Conversation

Licini
Copy link
Contributor

@Licini Licini commented Apr 29, 2024

Triggered by discussion of #1344 and #1248

I'm more on the side of allowing __rmul__, since a lot of people have the habit of writing equations in such order. On implementation level it would be also tricky to show a warning to say that we don't allow it (in that case __rmul__ is anyway needed).

Also adding support for element-wise multiplication and division for vector * vector, aligning with other libraries like Numpy, Tensorflow, Pytorch, R ... (In MATLAB this returns dot product tho)

The PR is created more for discussion purpose, let me know what you guys think

What type of change is this?

  • Bug fix in a backwards-compatible manner.
  • New feature in a backwards-compatible manner.
  • Breaking change: bug fix or new feature that involve incompatible API changes.
  • Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
  • I ran all tests on my computer and it's all green (i.e. invoke test).
  • I ran lint on my computer and there are no errors (i.e. invoke lint).
  • I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

@gonzalocasas
Copy link
Member

I like this, and the implementation looks good to me. I'm not sure how to assess correctly the potential side-effects of this though.

if isinstance(other, (int, float)):
return Vector(self.x * other, self.y * other, self.z * other)
elif isinstance(other, Vector):
return Vector(self.x * other[0], self.y * other[1], self.z * other[2])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if other is Vector, why not other.x etc?
also, this is the same as return self.dot(other)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are very right.... another mistake of mine not careful about what copilot suggested! But this is not same with dot product tho, this returns element-wise multiplication, the result is still of a vector, not the sum of them

src/compas/geometry/vector.py Outdated Show resolved Hide resolved
src/compas/geometry/vector.py Show resolved Hide resolved
@Licini
Copy link
Contributor Author

Licini commented May 7, 2024

I like this, and the implementation looks good to me. I'm not sure how to assess correctly the potential side-effects of this though.

I found this explanation for these functions:


In Python, the method __add__() and __radd__() are both part of the data model used to define how objects can be added together. The priority between these two methods depends on the operands involved in the addition operation. Here's how it works:

__add__() Method:
This method is attempted first when you use the + operator. It is a method for the left-hand operand of the addition.
For instance, in the expression a + b, Python first tries to call a.__add__(b).
__radd__() Method:
This method is the "right-side" addition method. It is called if the left operand's __add__() method does not support the addition with the right operand (either it returns NotImplemented, or the left operand doesn't have an __add__() method at all).
For instance, if a.__add__(b) returns NotImplemented, Python will check if b has a __radd__() method and if so, it will call b.__radd__(a).
In summary, __add__() has priority over __radd__() as it is attempted first. If __add__() fails to return a valid result and does not raise an exception, then __radd__() is attempted.

I will expand the test a bit here, to see what happens when different types are multiplied/added together

@Licini
Copy link
Contributor Author

Licini commented May 10, 2024

@tomvanmele @gonzalocasas What do you guys think about this way? If other is not a number, then we try casting the input into a Vector using Vector(*other) , this can allow more flexibility for other type of iterables

@Licini Licini requested a review from tomvanmele May 10, 2024 16:02
@Licini
Copy link
Contributor Author

Licini commented May 10, 2024

As for possible side effects of these reverse operator functions, because we don't return NotImplemented, this means that we will force the program to use our vector operator functions as long as our Vector class shows up at either side of operator. Which is not a necessarily a bad thing no?

@Licini
Copy link
Contributor Author

Licini commented Jun 7, 2024

@tomvanmele Could you take a look again and this one and see if it can be merged?

src/compas/geometry/vector.py Outdated Show resolved Hide resolved
src/compas/geometry/vector.py Outdated Show resolved Hide resolved
def __truediv__(self, n):
return Vector(self.x / n, self.y / n, self.z / n)
def __mul__(self, other):
if isinstance(other, (int, float)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we don't mind the performance penalty here i am okay with it...

@Licini Licini merged commit 8d40e38 into main Jun 7, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants