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 Filter, MovingWindowFilter, RotationSpline. #230

Merged

Conversation

LolaSegura
Copy link
Contributor

@LolaSegura LolaSegura commented Aug 30, 2021

🎉 New feature

Goes on top of #221
Related to #101 #210

Summary

Adds Python interface for three math classes: Filter, MovingWindowFilter and RotationSPline. For each class a python test has been created.

Related issues and notes

Filter

The Filter.hh file contains various template classes that inherit from a Filter class, which is a template class itself. To create binding for this classes an instance of Filter<type> had to be declared previously.
There were some classes that inherit from an instance of another class. For example class OnePoleQuaternion : public OnePole<math::Quaterniond>. This wasn't being interpreted by swig as expected. The inherited methods that were not override, were not being created in the binding. The solution we implemented for this was define these classes inside the interface file without the inheritance and adding the methods that were not being created. A similar approach was taken in the SignalStats class (#220 ).

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 Gazebo 1️1️ Dependency of Gazebo classic version 11 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Aug 30, 2021
@francocipollone
Copy link
Contributor

This PR is on hold until #221 is merged.

@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #230 (2eb2bab) into ign-math6 (b54dce5) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           ign-math6     #230   +/-   ##
==========================================
  Coverage      99.40%   99.40%           
==========================================
  Files             66       66           
  Lines           6185     6185           
==========================================
  Hits            6148     6148           
  Misses            37       37           

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 b54dce5...2eb2bab. Read the comment docs.

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 francocipollone marked this pull request as ready for review August 30, 2021 15:50
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021
@francocipollone
Copy link
Contributor

I'll solve the conflicts after #221 is merged.

#include "ignition/math/Vector3.hh"
%}

%import "std_vector.i"
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this line fixes the compilation error for me. Maybe it's a known issue, but I had to touch python.i for it to recompile this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me take a look. It seemed to be working before merging this branch with ign-math6.

Copy link
Contributor

Choose a reason for hiding this comment

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

You were right. That import was causing issues because of the syntaxis: it should be include instead of import

The truth is that that import wasn't necessary given that std::vector isn't being used in the API, so that import statement wasn't needed at all. I pushed the fix

Signed-off-by: Franco Cipollone <[email protected]>
@francocipollone francocipollone force-pushed the LolaSegura/add_filter_moving-window-filter_rotation-spline branch from 6d6708d to 7ebbcc7 Compare September 17, 2021 16:15
Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters merged commit e311b3b into ign-math6 Sep 18, 2021
@scpeters scpeters deleted the LolaSegura/add_filter_moving-window-filter_rotation-spline branch September 18, 2021 05:09
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 Gazebo 1️1️ Dependency of Gazebo classic version 11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants