Skip to content

Adds python interface to PID and SemanticVersion.#229

Merged
chapulina merged 3 commits intoign-math6from
LolaSegura/add_pid_semantic-version
Sep 3, 2021
Merged

Adds python interface to PID and SemanticVersion.#229
chapulina merged 3 commits intoign-math6from
LolaSegura/add_pid_semantic-version

Conversation

@LolaSegura
Copy link
Copy Markdown
Contributor

@LolaSegura LolaSegura commented Aug 25, 2021

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

@LolaSegura
Adds python interface to PID and SemanticVersion.
00d06c9 
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
@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
Copy Markdown

codecov bot commented Aug 25, 2021 •
edited
Loading

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.

francocipollone
francocipollone approved these changes Aug 25, 2021
View reviewed changes
Copy link
Copy Markdown
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

@francocipollone francocipollone marked this pull request as ready for review August 25, 2021 21:37
@francocipollone
Copy link
Copy Markdown
Contributor

francocipollone commented Aug 25, 2021

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

@LolaSegura
Address comments
5e11bc7 
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
francocipollone
francocipollone approved these changes Aug 26, 2021
View reviewed changes
Copy link
Copy Markdown
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!

@francocipollone francocipollone mentioned this pull request Aug 26, 2021
Closed
45 tasks
@chapulina
merged from ign-math6
cff698e 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
chapulina
chapulina approved these changes Sep 3, 2021
View reviewed changes
Copy link
Copy Markdown
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
@francocipollone
Copy link
Copy Markdown
Contributor

francocipollone commented Sep 6, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scpeters scpeters Awaiting requested review from scpeters scpeters is a code owner

2 more reviewers

@chapulina chapulina chapulina approved these changes

@francocipollone francocipollone francocipollone approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@LolaSegura @francocipollone @chapulina @j-rivero @osrf-triage