Skip to content
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

Adds python interface to RollingMean, Color and Spline #218

Merged
merged 17 commits into from
Sep 8, 2021

Conversation

LolaSegura
Copy link
Contributor

@LolaSegura LolaSegura commented Aug 10, 2021

🎉 New feature

Goes on top of #216.

Related to #101 #210

Summary

Adds Python interface for three math classes: RollingMean, Color, Spline. For each class a python test has been created.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Aug 10, 2021
@codecov
Copy link

codecov bot commented Aug 10, 2021

Codecov Report

Merging #218 (9e6df0f) into ign-math6 (b3804bf) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           ign-math6     #218   +/-   ##
==========================================
  Coverage      99.22%   99.22%           
==========================================
  Files             66       66           
  Lines           6162     6162           
==========================================
  Hits            6114     6114           
  Misses            48       48           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3804bf...9e6df0f. Read the comment docs.

@francocipollone francocipollone changed the title Lola segura/python interface Adds python interfaco to RollingMean, Color and Spline Aug 10, 2021
@francocipollone francocipollone changed the title Adds python interfaco to RollingMean, Color and Spline Adds python interface to RollingMean, Color and Spline Aug 10, 2021
@LolaSegura LolaSegura force-pushed the LolaSegura/python_interface branch from aaaa60a to 895eb1e Compare August 11, 2021 15:34
@LolaSegura
Copy link
Contributor Author

General comments

Given that there is no assignment in python the operator=() is ignored by swig. So the only way left to copy elements of the custom classes is by using the copy constructor.

The operator[]() was being ignored as well and this warning was shown when trying to build the ignition-math6 module Warning 389: operator[] ignored (consider using %extend). So as suggested to implement the indexation method we decided to create an extension.

The operator<<() had to be implemented using an %extend modifier as well. Also, for it to work correctly the std_string.i file had to be included.

Color

In C++ the getter methods are overloaded, so for example, getting the r element of a color could be done with both of this methods:
float R() const;
float &R();
But when creating the python interface swig interpreted this as if one method was shadowing the other. Given that a setter method is defined as well (void R(const float _r)) the decision was to implement only the first one.

@LolaSegura LolaSegura marked this pull request as ready for review August 12, 2021 18:59
@LolaSegura LolaSegura requested a review from scpeters as a code owner August 12, 2021 18:59
@francocipollone
Copy link
Contributor

@LolaSegura Please create another PR with the files related to Matrix3, Matrix4, Pose3, and Quaternion in order
to avoid this PR to grow this much and make it easier to review.

@LolaSegura LolaSegura force-pushed the LolaSegura/python_interface branch 2 times, most recently from 71a152d to b03e678 Compare August 18, 2021 18:15
@LolaSegura LolaSegura force-pushed the LolaSegura/python_interface branch from b03e678 to c0407ad Compare August 20, 2021 14:31
Copy link
Contributor

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

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

Nice! Just a few comments

src/python/Color.i Show resolved Hide resolved
src/python/Color.i Outdated Show resolved Hide resolved
src/python/Color_TEST.py Show resolved Hide resolved
src/python/RollingMean.i Outdated Show resolved Hide resolved
#endif
};
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Add an extra empty line at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

src/python/Spline.i Outdated Show resolved Hide resolved
src/python/Spline.i Outdated Show resolved Hide resolved
src/python/Spline_TEST.py Outdated Show resolved Hide resolved
Signed-off-by: LolaSegura <[email protected]>
Signed-off-by: LolaSegura <[email protected]>
Copy link
Contributor

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

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

LGTM!

@francocipollone
Copy link
Contributor

@scpeters @chapulina It is ready for an extra review!

@scpeters
Copy link
Member

I just resolved conflicts after merging #220

src/python/CMakeLists.txt Outdated Show resolved Hide resolved
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021
@scpeters scpeters merged commit c3ab4dc into ign-math6 Sep 8, 2021
@scpeters scpeters deleted the LolaSegura/python_interface branch September 8, 2021 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants