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 PID and SemanticVersion. #229

Merged
merged 3 commits into from
Sep 3, 2021

Conversation

LolaSegura
Copy link
Contributor

Signed-off-by: LolaSegura lsegura@ekumenlabs.com

🎉 New feature

Related to #101 #210

Summary

Adds Python interface for two math classes: PID, SemanticVersion. For each class a python test has been created.

Related issues and notes

PID

  • A copy constructor was added to the python class. The PID C++ class didn't have one defined, and, because on python the operator= is not supported, there was no way to copy two variables of type PID.
  • The Update(const double _error, const std::chrono::duration<double> &_dt) method received a variable of type std::chrono::duration. To give support to this method in python a new update function was defined using the %extend command. This new update method receives a variable of type double double Update(const double error, double dt).
  • The void Errors(double &_pe, double &_ie, double &_de) const; method changed the values of the arguments inside it. For native types swig has a way to handle this using the %apply <type> *OUTPUT {} directive. So in python this method can be used as [pe, ie, de] = pid.errors()
  • The %rename command is failing on adding and under case when the variable name is conformed by one letter and a world, for example, PGain() was being renamed as pgain() instead of p_gain(). So this cases where contemplated using an specific %rename tag.

SemanticVersion

  • The std_string.i module was added in order to work with std::string.

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

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
@osrf-triage osrf-triage added this to Inbox in Core development Aug 25, 2021
@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 25, 2021
@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #229 (cff698e) into ign-math6 (3eae090) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           ign-math6     #229   +/-   ##
==========================================
  Coverage      99.21%   99.21%           
==========================================
  Files             65       65           
  Lines           6089     6089           
==========================================
  Hits            6041     6041           
  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 3eae090...cff698e. 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, just some nits

src/python/PID.i Outdated Show resolved Hide resolved
src/python/PID.i Outdated Show resolved Hide resolved
src/python/SemanticVersion.i Outdated Show resolved Hide resolved
Core development automation moved this from Inbox to In review Aug 25, 2021
@francocipollone francocipollone marked this pull request as ready for review August 25, 2021 21:37
@francocipollone
Copy link
Contributor

I've removed the draft label. @LolaSegura Let me know if there is something that is missing

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
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
@scpeters @chapulina It is ready for a review!

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM!

The Update(const double _error, const std::chrono::duration &_dt) method received a variable of type std::chrono::duration. To give support to this method in python a new update function was defined using the %extend command. This new update method receives a variable of type double double Update(const double error, double dt).

I think this is ok for now, but maybe in the future we should look into using python time to be more expressive?

@chapulina chapulina enabled auto-merge (squash) September 3, 2021 17:01
@chapulina chapulina merged commit b4b6d23 into ign-math6 Sep 3, 2021
@chapulina chapulina deleted the LolaSegura/add_pid_semantic-version branch September 3, 2021 17:14
Core development automation moved this from In review to Done Sep 3, 2021
@francocipollone
Copy link
Contributor

LGTM!

The Update(const double _error, const std::chrono::duration &_dt) method received a variable of type std::chrono::duration. To give support to this method in python a new update function was defined using the %extend command. This new update method receives a variable of type double double Update(const double error, double dt).

I think this is ok for now, but maybe in the future we should look into using python time to be more expressive?

Agree. I think that we should provide a conversion from std::chrono to python time. Sadly, swig doesn't provide bindings as it does for std::string(std_string.p) or std::map (std_map.i) so probably creating a std_chrono.i might be useful to have, keeping in mind that there is this other class, Stopwatch, that also uses std::chrono extensively.

@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 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.

None yet

3 participants