-
Notifications
You must be signed in to change notification settings - Fork 110
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
Vector multiplications #1347
Conversation
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. |
src/compas/geometry/vector.py
Outdated
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]) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
I found this explanation for these functions:
I will expand the test a bit here, to see what happens when different types are multiplied/added together |
@tomvanmele @gonzalocasas What do you guys think about this way? If |
As for possible side effects of these reverse operator functions, because we don't return |
@tomvanmele Could you take a look again and this one and see if it can be merged? |
def __truediv__(self, n): | ||
return Vector(self.x / n, self.y / n, self.z / n) | ||
def __mul__(self, other): | ||
if isinstance(other, (int, float)): |
There was a problem hiding this comment.
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...
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?
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.CHANGELOG.md
file in theUnreleased
section under the most fitting heading (e.g.Added
,Changed
,Removed
).invoke test
).invoke lint
).compas.datastructures.Mesh
.