-
Notifications
You must be signed in to change notification settings - Fork 677
[robotpy] Add wpimath/geometry tests #8460
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
base: 2027
Are you sure you want to change the base?
[robotpy] Add wpimath/geometry tests #8460
Conversation
virtuald
left a comment
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.
I can't actually review the tests in detail, but they look sane. If they pass then it's probably good enough.
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.
Isn't this missing an equivalent for Rotation2dTest? (I'm not in a good position to say whether we want to block this PR on adding that in, since the PR comment does acknowledge that new tests might be missing. I will say though that it's really weird that Rotation2dTest would be missing, since that's been around for a really long time.)
Its the only test that already existed in robotpy (for the geometry subfolder). It already covers all the things the C++ tests do, so nothing needed to be added/modified. |
Ah, thanks, I missed that!
Sorry if I'm missing something again, but isn't it missing an equivalent to the |
|
|
||
| def test_composition(): | ||
| initial = Pose2d(x=1, y=2, rotation=Rotation2d.fromDegrees(45)) | ||
| transform1 = Transform2d(Translation2d(x=5, y=0), rotation=Rotation2d.fromDegrees(5)) |
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.
FWIW the order of the params for translations are probably obvious enough that I expect teams won't use keyword arguments to construct translations, transforms and poses. Not a big deal though.
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.
I just did a big regex find and replace in a lot of places. I can stop mod'ing that in the future prs
auscompgeek
left a comment
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.
LGTM
Used Gemini to convert the C++ tests in
wpimath/.../geometryto pytest. For ease of use required a small change to add attributes to theQuaternionbinding.I did this conversion months ago (before robotpy landed and before the reorg), so new tests might be missing. At least its better than nothing 🤷
Broke the huge pr that does almost everything into parts so its easier to review.