Skip to content

buffer_protocol for vectors and matrices in python bindings#524

Merged
arjo129 merged 8 commits intogazebosim:mainfrom
unjambonakap:gz-math7
Aug 25, 2024
Merged

buffer_protocol for vectors and matrices in python bindings#524
arjo129 merged 8 commits intogazebosim:mainfrom
unjambonakap:gz-math7

Conversation

@unjambonakap
Copy link
Copy Markdown
Contributor

@unjambonakap unjambonakap commented Mar 11, 2023 •
edited by mjcarroll
Loading

This is an extension/takeover of #498

Summary

This change enables the buffer_protocol for VectorX and MatrixYxY so that memoryiew and numpy.array can be used on them

np.array(math7.Vector3())
memoryview(math7.Matrix3d())

list(math7.Vector2d())
>>> math7.Quaterniond().xyzw() # == [0.0, 0.0, 0.0, 1.0]

Additionally:

  • IndexError is raised on invalid vector getitem/setitem access. This way list(vector) works correctly instead of spinning
  • A convenience Quaternion.xyzw() -> list is added

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@github-actions github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Mar 11, 2023
ahcorde
ahcorde requested changes Mar 13, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Do you mind to sign the commit?

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 13, 2023 •
edited
Loading

Codecov Report

Merging #524 (f1eeaf4) into gz-math7 (1e84669) will not change coverage.
The diff coverage is n/a.

❗ Current head f1eeaf4 differs from pull request most recent head 4907781. Consider uploading reports for the commit 4907781 to get more accurate results

@@            Coverage Diff            @@
##           gz-math7     #524   +/-   ##
=========================================
  Coverage     99.70%   99.70%           
=========================================
  Files            77       77           
  Lines          7026     7026           
=========================================
  Hits           7005     7005           
  Misses           21       21
Impacted Files Coverage Δ
include/gz/math/Matrix3.hh 100.00% <ø> (ø)
include/gz/math/Matrix4.hh 100.00% <ø> (ø)
include/gz/math/Matrix6.hh 100.00% <ø> (ø)
include/gz/math/Vector2.hh 100.00% <ø> (ø)
include/gz/math/Vector3.hh 100.00% <ø> (ø)
include/gz/math/Vector4.hh 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@unjambonakap unjambonakap requested review from ahcorde and removed request for adityapande-1995 and scpeters March 13, 2023 11:27
@unjambonakap
Copy link
Copy Markdown
Contributor Author

unjambonakap commented Mar 13, 2023

Tests are failing because of lack of numpy. Do you prefer to add numpy to the test environment or introducing this dependency is undesirable and I should rewrite the test to do away with numpy?

ahcorde
ahcorde previously requested changes Mar 14, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

you can add the dependency here https://github.com/gazebosim/gz-math/blob/gz-math7/.github/ci/packages.apt

@unjambonakap
Copy link
Copy Markdown
Contributor Author

unjambonakap commented Mar 14, 2023

you can add the dependency here https://github.com/gazebosim/gz-math/blob/gz-math7/.github/ci/packages.apt

Done, thanks for the pointer 🙏

@unjambonakap
Copy link
Copy Markdown
Contributor Author

unjambonakap commented Mar 16, 2023

@ahcorde do you have additional pointers on why tests are failing? In particular, homebrew is missing numpy

@mjcarroll
Copy link
Copy Markdown

mjcarroll commented Mar 20, 2023

do you have additional pointers on why tests are failing? In particular, homebrew is missing numpy

We generally don't add dependencies to stable releases. If you could, retarget this to main, and we can work on landing it there first with the additional dependencies.

@azeey
Copy link
Copy Markdown
Contributor

azeey commented Mar 20, 2023

We generally don't add dependencies to stable releases. If you could, retarget this to main, and we can work on landing it there first with the additional dependencies.

Agreed. Hopefully we can backport to gz-math7 without the numpy tests once it lands on main.

@willstott101
Copy link
Copy Markdown

willstott101 commented May 4, 2023 •
edited
Loading

Cool, thanks for taking this over.

Perhaps the comments could help explain a bit more what's going on/what the alternatives are?

  /// \brief Underlying data pointer
  /// \remarks This method is intended for python bindings (numpy).
  /// \remarks The bounds-checking array subscript operator is much preferred for element access from C++.
  /// \return A pointer to the underlying data array.

@willstott101 willstott101 mentioned this pull request May 4, 2023
Closed
8 tasks
@azeey azeey added beta Targeting beta release of upcoming collection and removed beta Targeting beta release of upcoming collection labels Jul 31, 2023
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
@arjo129
Copy link
Copy Markdown
Contributor

arjo129 commented Aug 8, 2024

Hi @unjambonakap , was wondering what the status on this was. Do we intend shipping this change with ionic (code freeze 26/8)? If we can't come to a consensus by then I will remove the beta label. For starters would it be possible for you to resolve the merge conflicts.

@unjambonakap unjambonakap force-pushed the gz-math7 branch 3 times, most recently from 4f123b2 to 462b040 Compare August 11, 2024 22:24
@arjo129
Copy link
Copy Markdown
Contributor

arjo129 commented Aug 12, 2024

Also as this PR breaks API can we plese target main and not gz-math7

@unjambonakap unjambonakap changed the base branch from gz-math7 to main August 12, 2024 10:40
@unjambonakap
buffer_protocol for vectors and matrices
f5a925a 
Also IndexError is raised on invalid vector __getitem__/__setitem__
access.
This way list(vector) works correctly instead of spinning

Signed-off-by: Benoit Maurin <maurinbe@gmail.com>
@unjambonakap
Add numpy package to CI
4e152f4 
Signed-off-by: Benoit Maurin <maurinbe@gmail.com>
@unjambonakap
Update comment + no fail tests if numpy not avail
4b08a5c 
- make Data() method comment clearer as per suggestion
- tests will succeed if numpy dependency is not available

Signed-off-by: Benoit Maurin <maurinbe@gmail.com>
@unjambonakap
Switch to static constexpr to fix capture issue
a45a76f 
Signed-off-by: Benoit Maurin <maurinbe@gmail.com>
@unjambonakap
Remove distutils dependency
948d04e 
Signed-off-by: Benoit Maurin <maurinbe@gmail.com>
@azeey azeey added 🏛️ ionic Gazebo Ionic and removed 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Aug 12, 2024
arjo129
arjo129 requested changes Aug 19, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

Thanks for these changes. I tested them and they mostly look good to me. There are some minor stylistic improvements that need to be made before I finally ✔️ them.

@azeey azeey added the Breaking change Breaks API, ABI or behavior. Must target unstable version. label Aug 20, 2024
@azeey
Copy link
Copy Markdown
Contributor

azeey commented Aug 22, 2024

@unjambonakap any chance you can respond to the feedback in the next couple days? Since gz-math is a lower level library in the Gazebo stack, we'd like to wrap up PRs earlier to make sure there is enough time to test them before the code freeze (August 28th).

Addisu Z. Taddese added 2 commits August 23, 2024 15:11
@unjambonakap
Copy link
Copy Markdown
Contributor Author

unjambonakap commented Aug 23, 2024 via email

@unjambonakap
Copy link
Copy Markdown
Contributor Author

unjambonakap commented Aug 23, 2024 via email

@azeey
Copy link
Copy Markdown
Contributor

azeey commented Aug 23, 2024

I didn't address some of the feedback. Mainly, I think it would be good to add c++ tests.

@unjambonakap
Add C++ tests for Data() and fix py file rename
6017ee7 
Signed-off-by: Benoit Maurin <maurinbe@gmail.com>
arjo129
arjo129 previously approved these changes Aug 25, 2024
View reviewed changes
@arjo129 arjo129 dismissed their stale review August 25, 2024 04:02

Haven't completely tested yet.

arjo129
arjo129 approved these changes Aug 25, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this. LGTM.

@unjambonakap unjambonakap requested a review from ahcorde August 25, 2024 19:53
@arjo129 arjo129 enabled auto-merge (squash) August 25, 2024 23:23
@arjo129 arjo129 dismissed ahcorde’s stale review August 25, 2024 23:25

In the interest of time, lets get this merged. @unjambonakap has already addressed the said issues.

@arjo129 arjo129 merged commit 99398fa into gazebosim:main Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arjo129 arjo129 arjo129 approved these changes

@ahcorde ahcorde Awaiting requested review from ahcorde

Assignees

No one assigned

Labels

beta Targeting beta release of upcoming collection Breaking change Breaks API, ABI or behavior. Must target unstable version. 🏛️ ionic Gazebo Ionic

Projects

Core development
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@unjambonakap @mjcarroll @azeey @willstott101 @arjo129 @ahcorde @osrf-triage