Adds Line2, Line3, SignalStats, Temperature python interface by WagnerMarcos · Pull Request #220 · gazebosim/gz-math

WagnerMarcos · 2021-08-12T17:14:47Z

🎉 New feature

Goes on top of #216.

Related to #101 #210

Summary

Adds Python interface for three math classes: Line2, SignalStats, Temperature. For each class a python test has been created.

Related issues and notes

Line2

Collinear() and Parallel() in ign-math was not working properly as the call to equal() was receiving (double, int, double) argument types, having a 0 with static cast passed as second argument, but always having doubles for the first and third arguments.. For this, a 0. is now being passed.
Instead of copying using the assignment operator = in python for the tests, the copy constructor was used.
To offer the operator[] functionality in python, the class had to be extended in the interface file (.i file) adding __getitem__(), calling the operator[] inside.
To offer the serialization functionality in python, the class had to be extended in the interface file (.i file) adding __str__(), calling the operator<< inside.

Temperature

To offer the serialization functionality in python, the class had to be extended in the interface file (.i file) adding __str__(), calling the operator<< inside.
Setting a new temperature value with the assignment operator isn't possible in python, so the constructor is being called in the test where that was being used.
The stream extraction operator hasn't been implemented in python to deserialize a string into an object.
The friend Temperature operator+(double _t, const Temperature &_temp) and similar operators haven't been implemented in SWIG because no simple implementation was found. So it won't be possible to do temp1 = 20.0 + temp2.

SignalStats

This contains various classes defined in the same interface file.

SignalStats class

A template for the map had to be created for it to be used as dictionaries in python.
The copy constructor was used instead of the assignment operator in python tests.

SignalStatistic class and its inherited classes

The copy functionality couldn't be done for the inherited classes, as the copy constructor was defined in the parent class, and in python it isn't assigning properly the class. A useful workaround in SWIG is yet to be found.

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

codecov · 2021-08-12T17:15:12Z

Codecov Report

Merging #220 (a905b63) into ign-math6 (23bcb08) will not change coverage.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           ign-math6     #220   +/-   ##
==========================================
  Coverage      99.21%   99.21%           
==========================================
  Files             65       65           
  Lines           6089     6089           
==========================================
  Hits            6041     6041           
  Misses            48       48

Impacted Files	Coverage Δ
include/ignition/math/Line2.hh	`100.00% <100.00%> (ø)`

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 23bcb08...8536f32. Read the comment docs.

Signed-off-by: Marcos Wagner <wagnermarcos@ekumenlabs.com>

WagnerMarcos · 2021-08-24T19:07:55Z

It was necessary to add the -Wno-class-memaccess flag to suppress the -Wclass-memaccess warning for the python target as when the std_map from SWIG is used with SWIG 3.0.12 in the SignalStats file, we were getting the following:

Click to see output

/ws/build/ignition-math6/lib/python/pythonPYTHON_wrap.cxx: In instantiation of 'static Type swig::traits_as<Type, swig::pointer_category>::as(PyObject*, bool) [with Type = std::pair<std::__cxx11::basic_string<char>, double>; PyObject = _object]':
/ws/build/ignition-math6/lib/python/pythonPYTHON_wrap.cxx:4153:64:   required from 'Type swig::as(PyObject*, bool) [with Type = std::pair<std::__cxx11::basic_string<char>, double>; PyObject = _object]'
/ws/build/ignition-math6/lib/python/pythonPYTHON_wrap.cxx:4720:20:   required from 'swig::SwigPySequence_Ref<T>::operator T() const [with T = std::pair<std::__cxx11::basic_string<char>, double>]'
/ws/build/ignition-math6/lib/python/pythonPYTHON_wrap.cxx:4780:14:   required from 'swig::SwigPySequence_ArrowProxy<T> swig::SwigPySequence_InputIterator<T, Reference>::operator->() const [with T = std::pair<std::__cxx11::basic_string<char>, double>; Reference = const swig::SwigPySequence_Ref<std::pair<std::__cxx11::basic_string<char>, double> >]'
/ws/build/ignition-math6/lib/python/pythonPYTHON_wrap.cxx:5464:25:   required from 'void swig::assign(const SwigPySeq&, std::map<K, T, Compare, Alloc>*) [with SwigPySeq = swig::SwigPySequence_Cont<std::pair<std::__cxx11::basic_string<char>, double> >; K = std::__cxx11::basic_string<char>; T = double; Compare = std::less<std::__cxx11::basic_string<char> >; Alloc = std::allocator<std::pair<const std::__cxx11::basic_string<char>, double> >]'
/ws/build/ignition-math6/lib/python/pythonPYTHON_wrap.cxx:5337:12:   required from 'static int swig::traits_asptr_stdseq<Seq, T>::asptr(PyObject*, swig::traits_asptr_stdseq<Seq, T>::sequence**) [with Seq = std::map<std::__cxx11::basic_string<char>, double>; T = std::pair<std::__cxx11::basic_string<char>, double>; PyObject = _object; swig::traits_asptr_stdseq<Seq, T>::sequence = std::map<std::__cxx11::basic_string<char>, double>]'
/ws/build/ignition-math6/lib/python/pythonPYTHON_wrap.cxx:5480:64:   required from 'static int swig::traits_asptr<std::map<_Key, _Tp, _Compare, _Allocator> >::asptr(PyObject*, swig::traits_asptr<std::map<_Key, _Tp, _Compare, _Allocator> >::map_type**) [with K = std::__cxx11::basic_string<char>; T = double; Compare = std::less<std::__cxx11::basic_string<char> >; Alloc = std::allocator<std::pair<const std::__cxx11::basic_string<char>, double> >; PyObject = _object; swig::traits_asptr<std::map<_Key, _Tp, _Compare, _Allocator> >::map_type = std::map<std::__cxx11::basic_string<char>, double>]'
/ws/build/ignition-math6/lib/python/pythonPYTHON_wrap.cxx:4045:37:   required from 'int swig::asptr(PyObject*, Type**) [with Type = std::map<std::__cxx11::basic_string<char>, double>; PyObject = _object]'
/ws/build/ignition-math6/lib/python/pythonPYTHON_wrap.cxx:34234:154:   required from here
/ws/build/ignition-math6/lib/python/pythonPYTHON_wrap.cxx:4128:8: warning: 'void* memset(void*, int, size_t)' clearing an object of type 'struct std::pair<std::__cxx11::basic_string<char>, double>' with no trivial copy-assignment; use assignment instead [-Wclass-memaccess]
  memset(v_def,0,sizeof(Type));
  ~~~~~~^~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/c++/8/bits/stl_algobase.h:64,
                 from /usr/include/c++/8/bits/specfun.h:45,
                 from /usr/include/c++/8/cmath:1892,
                 from /usr/include/c++/8/math.h:36,
                 from /usr/include/python3.6m/pyport.h:194,
                 from /usr/include/python3.6m/Python.h:53,
                 from /ws/build/ignition-math6/lib/python/pythonPYTHON_wrap.cxx:173:
/usr/include/c++/8/bits/stl_pair.h:208:12: note: 'struct std::pair<std::__cxx11::basic_string<char>, double>' declared here
     struct pair
            ^~~~

This issue is not happening when compiling with SWIG 4.0, but in order to support SWIG 3.0, and as we couldn't find a fix for this at the moment, we decided to suppress it.

francocipollone

LGTM, just a couple of nits

src/python/Line2.i

src/python/Line2_TEST.py

src/python/Line3.i

src/python/Line3_TEST.py

src/python/SignalStats.i

src/python/SignalStats_TEST.py

scpeters · 2021-08-24T23:25:17Z

src/python/python.i

+%include Line2.i
+%include Line3.i
+%include SignalStats.i
+%include Temperature.i


nit: can these be alphabetized or do they need to be ordered by dependencies?

do they need to be ordered by dependencies?

That's correct

Signed-off-by: Marcos Wagner <wagnermarcos@ekumenlabs.com>

francocipollone

LGTM!

@scpeters @chapulina It is ready for another review!

Signed-off-by: Marcos Wagner <wagnermarcos@ekumenlabs.com>

github-actions bot added 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Aug 12, 2021

WagnerMarcos requested a review from francocipollone August 12, 2021 17:15

WagnerMarcos changed the title ~~Adds Line2, SignalStats, Temperature python interface~~ Adds Line2, Line3, SignalStats, Temperature python interface Aug 13, 2021

WagnerMarcos marked this pull request as ready for review August 13, 2021 18:47

WagnerMarcos requested a review from scpeters as a code owner August 13, 2021 18:47

francocipollone force-pushed the wagnermarcos/temp_line2_signalstats_py_interface branch from 474070f to 8456acf Compare August 18, 2021 13:29

francocipollone mentioned this pull request Aug 19, 2021

Python interface: Renames methods to match PEP8 style #226

Merged

8 tasks

WagnerMarcos mentioned this pull request Aug 23, 2021

Line2.Collinear function not work right. #213

Closed

WagnerMarcos added 3 commits August 24, 2021 11:42

Adds Line2, SignalStats, Temperature python interface

c97f3e4

Signed-off-by: Marcos Wagner <wagnermarcos@ekumenlabs.com>

Adding Line3 class with its test

3fefff8

Signed-off-by: Marcos Wagner <wagnermarcos@ekumenlabs.com>

Adding %rename tag to interface files to match PEP8

31be8f8

Signed-off-by: Marcos Wagner <wagnermarcos@ekumenlabs.com>

WagnerMarcos force-pushed the wagnermarcos/temp_line2_signalstats_py_interface branch from 8456acf to 31be8f8 Compare August 24, 2021 18:52

francocipollone approved these changes Aug 24, 2021

View reviewed changes

scpeters reviewed Aug 24, 2021

View reviewed changes

Applying alphasort to include and snake_case to variables

34be3d6

Signed-off-by: Marcos Wagner <wagnermarcos@ekumenlabs.com>

francocipollone approved these changes Aug 25, 2021

View reviewed changes

Fixing a typo and trailing spaces

8536f32

Signed-off-by: Marcos Wagner <wagnermarcos@ekumenlabs.com>

francocipollone mentioned this pull request Aug 26, 2021

Documentation, Style and scripting interfaces update #101

Closed

45 tasks

This was referenced Aug 30, 2021

Adds python interface to Filter, MovingWindowFilter, RotationSpline. #230

Merged

Adds python interface to triangle. #231

Merged

WagnerMarcos mentioned this pull request Aug 30, 2021

Adds python interface for Kmeans and Vector3Stats #232

Merged

8 tasks

scpeters approved these changes Aug 30, 2021

View reviewed changes

scpeters merged commit 3ea6229 into ign-math6 Aug 30, 2021

scpeters deleted the wagnermarcos/temp_line2_signalstats_py_interface branch August 30, 2021 19:02

scpeters mentioned this pull request Aug 30, 2021

Adds python interface to RollingMean, Color and Spline #218

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Line2, Line3, SignalStats, Temperature python interface#220

Adds Line2, Line3, SignalStats, Temperature python interface#220
scpeters merged 5 commits intoign-math6from
wagnermarcos/temp_line2_signalstats_py_interface

WagnerMarcos commented Aug 12, 2021 •

edited

Loading

Uh oh!

codecov bot commented Aug 12, 2021 •

edited

Loading

Uh oh!

WagnerMarcos commented Aug 24, 2021

Uh oh!

francocipollone left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scpeters Aug 24, 2021

Uh oh!

francocipollone Aug 25, 2021

Uh oh!

francocipollone left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

WagnerMarcos commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎉 New feature

Summary

Related issues and notes

Line2

Temperature

SignalStats

SignalStats class

SignalStatistic class and its inherited classes

Checklist

Uh oh!

codecov bot commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

WagnerMarcos commented Aug 24, 2021

Uh oh!

francocipollone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scpeters Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

francocipollone Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

francocipollone left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

WagnerMarcos commented Aug 12, 2021 •

edited

Loading

codecov bot commented Aug 12, 2021 •

edited

Loading