-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: like
method for projecting vector into the coordinate space of another vector + better type errors and hints
#426
feat: like
method for projecting vector into the coordinate space of another vector + better type errors and hints
#426
Conversation
Hi @jpivarski, @nsmith-, @iasonkrom, @lgray I've noticed that some operations require the vectors to be of different dimensions; for instance, Is there a specific set of operations that should error out when performed on vectors of different dimensionality, or maybe I should ask about a particular set of operations that should not error? The |
I would assume that those that require different dimensionality are fewer so.... the second option? Y'all may have better opinions though. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #426 +/- ##
==========================================
- Coverage 82.92% 82.81% -0.12%
==========================================
Files 96 96
Lines 11533 11557 +24
==========================================
+ Hits 9564 9571 +7
- Misses 1969 1986 +17 ☔ View full report in Codecov by Sentry. |
feat: error while out on operations on vectors of different geomtric dimensions + new `like` method
6025671
to
09b2de8
Compare
like
methodlike
method for projecting vector into the coordinate space of another vector + better type errors and hints
Some methods do require arguments of a particular type to be meaningful, such as |
Ah, this can be easily highlighted by updating the type signature, so I think an error in such situations will be redundant? --- a/src/vector/_methods.py
+++ b/src/vector/_methods.py
@@ -3818,7 +3818,7 @@ class Lorentz(Spatial, VectorProtocolLorentz):
return boost_p4.dispatch(self, p4)
def boost_beta3(
- self: SameVectorType, beta3: VectorProtocolSpatial
+ self: SameVectorType, beta3: MomentumProtocolSpatial
) -> SameVectorType:
from vector._compute.lorentz import boost_beta3 |
Thinking about this again, do we want strict checks in each method/operation or do we want strict checks only in the methods/operations that can accept every type of vector ( For instance, def add(self, other: VectorProtocol) -> VectorProtocol: Therefore, I have added a type check in this method. But, def cross(self, other: VectorProtocolSpatial) -> VectorProtocolSpatial: and the following works: In [1]: import vector
In [2]: vector.obj(x=0.1, y=0.2, z=0.3, t=10).cross(vector.obj(x=0.4, y=0.5, z=0.6, t=20))
Out[2]: VectorObject3D(x=-0.03, y=0.06, z=-0.030000000000000013) From an API design point of view, should we check the type of an argument even after providing a type hint? It feels redundant to me. Maybe the implementation of cc: @jpivarski |
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.
The new like
method is documented, but there should also be a short comment about it in the error messages that result from trying to add/subtract/compare vectors of different dimensionality, since the like
method is how users would fix those errors.
The exclusion of implicit projection/embedding and inclusion of a new like
method together form a single project of asking users for clarity about whether they want to project or embed. Some text like
x + y → x.like(y) + y or x + y.like(x)
would indicate where the user is being asked for their opinion.
README.md
Outdated
v1 - v2.like(v1), v1.like( | ||
v2 | ||
) - v2, v1 - v2.to_xyz(), v1.to_xy() - v2, v1 - v2.to_Vector3D( | ||
z=3 | ||
), v1.to_Vector2D() - v2 |
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.
This might be required for black-compliant formatting, but it makes what would be easy to read into something difficult to read.
Do you need the commas?
v1 - v2.like(v1), v1.like( | |
v2 | |
) - v2, v1 - v2.to_xyz(), v1.to_xy() - v2, v1 - v2.to_Vector3D( | |
z=3 | |
), v1.to_Vector2D() - v2 | |
v1 - v2.like(v1) | |
v1.like(v2) - v2 | |
v1 - v2.to_xyz() | |
v1.to_xy() - v2 | |
v1 - v2.to_Vector3D(z=3) | |
v1.to_Vector2D() - v2 |
Should there be some explanatory text between these?
src/vector/_methods.py
Outdated
def _maybe_dimension_error( | ||
v1: VectorProtocol, v2: VectorProtocol, operation: str | ||
) -> None: | ||
"""Raises an error if the vectors are not of the same dimension.""" | ||
if dim(v1) != dim(v2): | ||
raise TypeError( | ||
f"""{v1!r} and {v2!r} do not have the same dimension; use | ||
|
||
a.like(b).{operation}(b) | ||
|
||
or | ||
|
||
a.{operation}(b.like(a)) | ||
|
||
or the binary operation equivalent to project or embed one of the vectors | ||
to match the other's dimensionality | ||
""" | ||
) |
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.
Thanks for the review, @jpivarski! The error message here displays the usage of the new like
method. Should this be changed in any way?
like
method for projecting vector into the coordinate space of another vector + better type errors and hintslike
method for projecting vector into the coordinate space of another vector + better type errors and hints
Re-requesting your review on this too, @jpivarski! |
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.
This is great! I approve!
Having a utility function for that error message is a good idea; it seems to be needed in a lot of methods.
Great, thanks for the review! |
Description
Xref: #422
Will rebase once #423 is merged.
Please describe the purpose of this pull request. Reference and link to any relevant issues or pull requests.
Checklist
$ pre-commit run --all-files
or$ nox -s lint
)$ pytest
or$ nox -s tests
)$ cd docs; make clean; make html
or$ nox -s docs
)$ pytest --doctest-plus src/vector/
or$ nox -s doctests
)Before Merging