tests: geometry (tier-1)#184
Conversation
jandom
added
the
safe-to-test
jandom
added
safe-to-test
jnwei
left a comment
jnwei
left a comment
There was a problem hiding this comment.
LGTM with some fixes requested for imports and a readability nit
| from .helpers import rigid as _rigid | ||
| from .helpers import rot_x as _rot_x | ||
| from .helpers import rot_z as _rot_z | ||
| from .helpers import v as _v |
There was a problem hiding this comment.
Prefer to use the explicit import path, rather than the relative path, e.g. openfold3.tests.utils.geometry.helpers
There was a problem hiding this comment.
switched, i also added a ruff rule to enforce this repo-wide, so a few other files were touched
There was a problem hiding this comment.
nit: It would be slightly more clear if we annotated the return types that are scalars (formerly Float) to with something like
def dot(self, other: Vec3Array) -> torch.Tensor: # return: scalar
There was a problem hiding this comment.
ah, this is something I still need to add
| recovered = rig.to_tensor() | ||
| assert torch.allclose(recovered, mat) | ||
|
|
||
| def test_from_tensor_4x4_round_trip(self): |
There was a problem hiding this comment.
since .from_tensor_4x4 and .to_tensor_4x4 is an alias of .from_tensor and .to_tensor, I think this test is redundant and could be removed
There was a problem hiding this comment.
Yup, good catch, thx
| ) | ||
|
|
||
|
|
||
| def rigid(rot: Rot3Array, tx: float, ty: float, tz: float) -> Rigid3Array: |
There was a problem hiding this comment.
nit - It would be a lot more readable if tx, ty, and tz were passed in as a tuple, rather than as 3 separate floats.
In the helper function, these values get converted into a Vector3D anyway.
| # =================================================================== | ||
|
|
||
|
|
||
| class TestScalarMultiply: |
There was a problem hiding this comment.
nit: Should probably include a test case for multiply by a scalar != 1 or 0
| from .helpers import v as _v | ||
| from .helpers import vb as _vb | ||
|
|
There was a problem hiding this comment.
Prefer direct imports rather than relative imports
jandom
added
safe-to-test
2 participants
Summary
Changes
Related Issues
Testing
Other Notes