Yaml extension for Eigen::Matrix and std::unordered_map #175

gilwoolee · 2017-04-16T20:46:04Z

Adds YAML parser for Eigen datatype, originally implemented by @mkoval . Copied from kenv. Allows operations such as node.as<Eigen::VectorXd>, node.as<Eigen::Isometry3d> , etc..

…rsonalrobotics/aikido into SimBarrettHandCommandExecutor

Duplicate changes in BarrettHandKinematicSimulationPositionCommandExecutor.

…cutor in HandPositionCommandExecutor. Adapted from https://github.com/personalrobotics/libherb/pull/20.

Make test_BarrettFinger* compile (although actual execution seems to fail). Make part of test_BarrettHand compile, although introducing `robot` as a parameter seems like it will make testing this difficult.

…rsonalrobotics/aikido into util/yaml_helper

mkoval

This generally looks good. I guess I shouldn't be surprised, since I wrote it!

A few minor comments, namely:

Add docstrings for the helper functions, e.g. aikido::util::deserialize and, if you're feeling energetic, the yaml-cpp template class specializations.
Add unit tests for converting to/from YAML.

Also a few points for discussion:

This file is a bit of a mess (as it was in muul). Does it make sense to split some of this into the detail directory? @gilwoolee @jslee02 does this make sense to you, and if so, which parts?
It would be nice to eliminate the dependency on boost::format. Is it worth replacing those calls with std::stringstream to do so?

mkoval · 2017-04-17T02:14:49Z

include/aikido/util/yaml_utils.hpp

+#include <vector>
+#include <iostream>
+#include <boost/format.hpp>
+#include <boost/make_shared.hpp>


Of these, I think only Eigen/Dense, yaml-cpp/yaml.h, and boost/format.hpp are actually used. We should remove the unused #include directives.

Actually, I'd also like to replace boost::format with std:;stringstream to eliminate that dependency as well.

mkoval · 2017-04-17T02:20:24Z

include/aikido/util/yaml_utils.hpp

+{
+    typedef Eigen::Matrix<_Scalar, _Rows, _Cols, _Options, _MaxRows, _MaxCols> MatrixType;
+    typedef typename MatrixType::Index Index;
+    typedef typename MatrixType::Scalar Scalar;


Nit: Prefer using aliases to typedefs.

mkoval · 2017-04-17T02:23:33Z

include/aikido/util/yaml_utils.hpp

+struct encode_impl<MatrixType, true> {
+  static Node encode(MatrixType const &matrix)
+  {
+    typedef typename MatrixType::Index Index;


Nit: Prefer using aliases to typedefs.

mkoval · 2017-04-17T02:25:17Z

include/aikido/util/yaml_utils.hpp

+inline void operator>>(Node const &node, T &value)
+{
+    value = node.as<T>();
+}


I'd like to remove this. If I remember correctly, I added this as a workaround to support both yaml-cpp versions 0.3 (which used the bitshift operators) and 0.5 (which used the as function).

mkoval · 2017-04-17T02:31:19Z

include/aikido/util/yaml_utils.hpp

+
+template <class _Scalar,
+          int _Rows, int _Cols, int _Options, int _MaxRows, int _MaxCols>
+inline void deserialize(


Missing docstring. I'm willing to let it slide for the convert that we specialize for yaml-cpp - but this is purely a helper function, so it should be properly documented. 😁

I believe we don't use this function out of template <...> struct convert. Could I just move this into it as you said?

mkoval · 2017-04-17T02:32:09Z

include/aikido/util/yaml_utils.hpp

+inline bool has_child(YAML::Node const &node, T const &key)
+{
+	return node[key].IsDefined();
+}


This function is not used, please delete it.

mkoval · 2017-04-17T02:32:51Z

include/aikido/util/yaml_utils.hpp

+
+template <class _Scalar, int Dim, int Mode, int _Options>
+inline void deserialize(YAML::Node const &node,
+                        Eigen::Transform<_Scalar, Dim, Mode, _Options> &pose)


Missing docstring. See above. 😉

mkoval · 2017-04-17T02:38:50Z

include/aikido/util/yaml_utils.hpp

+namespace detail {
+
+template <typename MatrixType, bool IsVectorAtCompileTime>
+struct encode_impl {


This is an internal helper class, so it deserves a docstring. 😁

The reason this exists is so we can use template specialization to switch between serializing vectors and serializing matrices based on the IsVectorAtCompileTime flag. This is a "nice to have" that lets us generate !Vector [1, 2, 3] instead of the syntactic travesty like !Matrix [[1], [2] ,[3]] when serializing an Eigen vector.

mkoval · 2017-04-17T02:40:37Z

include/aikido/util/yaml_utils.hpp

+  }
+};
+
+} //detail


Nit: We may want to move this helper code into the detail directory. @gilwoolee @jslee02 Thoughts?

jslee02 · 2017-04-17T16:41:06Z

This file is a bit of a mess (as it was in muul). Does it make sense to split some of this into the detail directory? @gilwoolee @jslee02 does this make sense to you, and if so, which parts?

I believe none of these functions needs to be exposed as public API. So all the code should be in detail I think. Also, we might mostly want to enable to use yaml with Eigen type. For this, what I would do is like:

move this whole file to detail directory
create yaml_cpp.cpp that includes both <yaml-cpp/yaml.h> and this file
encourage Aikido to use yaml_cpp.hpp all the time rather than <yaml-cpp/yaml.h>

More importantly, we already have this file in perception component in a different name. 😂

jslee02 · 2017-04-17T16:43:34Z

include/aikido/util/yaml_utils.hpp

+          int _Rows, int _Cols, int _Options, int _MaxRows, int _MaxCols>
+inline void deserialize(
+    YAML::Node const &node,
+    Eigen::Matrix<_Scalar, _Rows, _Cols, _Options, _MaxRows, _MaxCols> &matrix)


We might want to follow the suggestion of Eigen's "Writing Functions Taking Eigen Types as Parameters" here.

and add a test

coveralls · 2017-04-19T16:21:16Z

Coverage decreased (-0.02%) to 80.35% when pulling 3eb4b88 on util/yaml_helper into 2af5c55 on master.

mkoval

Question: It seems the yaml-cpp's convention is to return false when failed to decode rather than throwing an exception, but the decoder for Eigen::Matrix in this PR throws exceptions for the case. So I would like to change it to follow the convention.

I am conflicted. On one hand, I'd like to honor yaml-cpp's API convention as much as possible. On the other hand, it is hard to debug parsing errors when as returns false without a useful error message.

Additionally, the code currently only raises an exception if a node is tagged as a !Vector or !Matrix that cannot be interpreted as such. I would be more open to this change if we modified the conversion functions to not require those tags.

I believe that is possible with a small refactor. Instead of checking the tag, we'll simply check whether the node contains two layers of nested lists (a Matrix), one level of lists (a Vector), or another value (an error). I like this idea quite a bit because many people are not familiar with tags in YAML.

mkoval · 2017-04-21T01:22:17Z

include/aikido/util/detail/yaml_extension.hpp

+  }
+};
+
+} // namespace (anonymous)


Don't use anonymous namespaces in headers. I suggest putting this in a detail namespace instead.

I thought we use anonymous namespace for objects that are only used in the implementation, and use detail namespace for objects that are used somewhere out of the implementation but don't expect that is used by the user.

In most cases, the implementation is placed in a source file, but we have this decode in a header file because it's a template. I think we could consider putting this into a separate file with the name yaml_extension-impl.hpp instead.

I don't think we should ever put an anonymous namespace in a header file. A header file can be #includeed into more than one translation unit, so this will result in unnecessary duplication of the objects (in the best case) or difficult-to-diagnose bugs (in the worst case, e.g. possibly if the namespace contains static member or global variables).

I see the detail namespace as the logical equivalent of an anonymous namespace in the header file, i.e. it is an implementation detail of other functions in that file.

mkoval · 2017-04-21T01:24:04Z

include/aikido/util/detail/yaml_extension.hpp

+  {
+    if (node.Tag() == "Vector" || node.Tag() == "!Vector"
+        || node.Tag() == "Matrix"
+        || node.Tag() == "!Matrix")


I am pretty sure that we only need the set with the ! or the one without. If I remember correctly, I have both here because the behavior changed between yaml-cpp 0.3 and 0.5. Aikido only supports yaml-cpp 0.5, so that is a moot point.

Unfortunately, I don't remember which is used in 0.5. I'd like to do a quick test and eliminate the superfluous check.

I'm not sure either, but it seems the spec says ! should be used. So let's stick with that. 😅

mkoval · 2017-04-21T01:25:26Z

include/aikido/util/detail/yaml_extension.hpp

+
+    map.clear();
+    for (const auto& it : node)
+      map[it.first.as<_Key>()] = it.second.as<_Tp>();


What happens if as<>() fails? If we want to return false on failure, then we need to handle that case explicitly.

mkoval · 2017-04-21T01:25:55Z

include/aikido/util/detail/yaml_extension.hpp

+    if (!node.IsMap())
+      return false;
+
+    map.clear();


Nit: Call map.reserve(node.size()).

psigen · 2017-04-21T13:07:52Z

I do not think it is a good idea to introduce exceptions into the yaml-cpp API. This is going to screw up the type of failure that users are expecting to need to handle in parsing code.

And doesn't the yaml-cpp .as<> wrapper raise a TypedBadConversion if the internal converter returns false anyway? That exception will include additional information like where the conversion error happened and the type.

https://github.com/jbeder/yaml-cpp/blob/master/include/yaml-cpp/node/impl.h#L125

jslee02 · 2017-04-21T17:02:02Z

I agree what @mkoval and @psigen said. It seems we shouldn't introduce an exception for struct convert as yaml-cpp does and let .as<> raise a TypedBadConver exception when decode returns false.

Also, I like the idea not to use additional tag.

jslee02 · 2017-04-21T17:37:16Z

Hm, looking into more yaml-cpp code, I started to incline using exceptions because it throws exception anyway and .as<> throws exceptions with insufficient information of the error. It only shows where (line and column) the exception occurred but not what the exact cause. It only says "bad conversion".

As a consensus, I would like to use exceptions but yaml-cpp's custom exception object, YAML::Exception that allows us to pass the error message.

coveralls · 2017-04-21T19:43:56Z

Coverage decreased (-0.02%) to 80.35% when pulling e359fef on util/yaml_helper into 2af5c55 on master.

coveralls · 2017-04-21T20:12:58Z

Coverage remained the same at 80.374% when pulling c5ce755 on util/yaml_helper into 2af5c55 on master.

coveralls · 2017-04-22T19:06:18Z

Coverage remained the same at 79.163% when pulling e39ef72 on util/yaml_helper into 5d19317 on master.

mkoval · 2017-04-22T21:20:04Z

I agree with @jslee02's conclusion - let's keep exceptions, but switch from std::runtime_error to YAML::Exception. Like @jslee02 said, yaml-cpp generates terrible error messages.

Also, I would like to eliminate the need for the !Matrix and !Vector tags before merging this. @gilwoolee or @jslee02 could you take a shot at that?

jslee02 · 2017-04-22T22:05:10Z

I believe this PR is ready to merge.

One remained an issue but not a blocker is removing the requirement of tag for vector and matrix. I'll create an issue for this to implement this in the future.

coveralls · 2017-04-22T22:36:38Z

Coverage decreased (-0.01%) to 79.153% when pulling 5b39cb7 on util/yaml_helper into 5d19317 on master.

mkoval · 2017-04-22T23:45:41Z

include/aikido/util/detail/yaml_extension.hpp

+#else
+  DART_UNUSED(node);
+  return YAML::Mark::null_mark();
+#endif


Why is this #ifdef necessary? Can you add a comment explaining why?

mkoval · 2017-04-22T23:46:32Z

include/aikido/util/detail/yaml_extension.hpp

+          int _Cols,
+          int _Options,
+          int _MaxRows,
+          int _MaxCols>


@jslee02 Did you determine whether if or not we can replace this with template <class _Derived> struct<Eigen::MatrixBase<_Derived>>? I really liked that idea.

Yeah, I haven't figured it out yet, but it doesn't work for struct. 😞 Let's put this over for now.

coveralls · 2017-04-23T00:20:20Z

Coverage remained the same at 79.044% when pulling 9b437a9 on util/yaml_helper into 4af95ae on master.

coveralls · 2017-04-23T00:54:46Z

Coverage remained the same at 78.813% when pulling fe89b10 on util/yaml_helper into 4c08ac5 on master.

coveralls · 2017-04-23T01:23:42Z

Coverage remained the same at 78.813% when pulling fe89b10 on util/yaml_helper into 4c08ac5 on master.

gilwoolee and others added 23 commits April 12, 2017 15:24

Renaming, adding an abstract BarretHandPositionCommandExecutor class

8feb924

Adding -override

811a63b

Merge branch 'master' into SimBarrettHandCommandExecutor

dd96c10

Exposing step

83f69dd

Merge branch 'SimBarrettHandCommandExecutor' of https://github.com/pe…

cf8f104

…rsonalrobotics/aikido into SimBarrettHandCommandExecutor

Renaiming PositionCommandExecutor

9d9fb3d

Changing header files

648c2bf

Changing names and step function.

a38c65a

Merge branch 'master' into SimBarrettHandCommandExecutor

06dad13

Update simulation speed constants and matrix->vector methods.

c6462db

Add collisionDetector argument. Make collideWith an optional argument.

e16880e

Don't reset mCollideWith every time.

ce86f93

Simplify logic of optional collision detector and collision groups.

c90cd8d

Duplicate changes in BarrettHandKinematicSimulationPositionCommandExecutor.

Add ShapeFrames of original collision group to new collision group.

2d3a5bc

Make collisionDetector, collideWith optional.

d9d36fc

Instantiate FingerPositionCommandExecutors and FingerSpreadCommandExe…

3af8671

…cutor in HandPositionCommandExecutor. Adapted from https://github.com/personalrobotics/libherb/pull/20.

Clean up some documentation.

2174930

Remove parameter from step.

6ecb30e

Set mCollisionDetector when setting mCollideWith.

c3eb6f3

Make FingerCommandExecutors implement PositionCommandExecutor.

47a9852

Swap argument order in HandPositionExecutor.

bebb761

Partial test fixes.

f1d805f

Make test_BarrettFinger* compile (although actual execution seems to fail). Make part of test_BarrettHand compile, although introducing `robot` as a parameter seems like it will make testing this difficult.

Yaml parser

0be9ae8

gilwoolee requested a review from mkoval April 16, 2017 20:46

gilwoolee added 2 commits April 16, 2017 18:31

Adding time-reset when a new execute command is given.

fc7cd1c

Merge branch 'SimBarrettHandCommandExecutor' of https://github.com/pe…

57e404f

…rsonalrobotics/aikido into util/yaml_helper

mkoval requested changes Apr 17, 2017

View reviewed changes

jslee02 self-requested a review April 17, 2017 02:48

jslee02 suggested changes Apr 17, 2017

View reviewed changes

Rename test_yaml_eigen_extension to test_yaml_extension

3eb4b88

and add a test

jslee02 approved these changes Apr 20, 2017

View reviewed changes

jslee02 changed the title ~~Yaml parser~~ Yaml extension for Eigen::Matrix and std::unordered_map Apr 20, 2017

mkoval requested changes Apr 21, 2017

View reviewed changes

jslee02 added 2 commits April 21, 2017 15:15

Fix typos in version.hpp.in

7a8f2c6

Use YAML::RepresentationException instead of std::exception

e359fef

Fix style

c5ce755

Merge branch 'master' into util/yaml_helper

e39ef72

Use detail namespace instead of anonymous namespace

5e4b783

Make function inline-ed that is defined in header

5b39cb7

jslee02 mentioned this pull request Apr 22, 2017

Consider removing tag requirements for vector and matrix in the yaml parser #188

Closed

Merge branch 'master' into util/yaml_helper

9b437a9

mkoval approved these changes Apr 22, 2017

View reviewed changes

jslee02 and others added 2 commits April 22, 2017 20:25

Add comment

151b7e6

Merge branch 'master' into util/yaml_helper

fe89b10

mkoval merged commit a806042 into master Apr 23, 2017

mkoval deleted the util/yaml_helper branch April 23, 2017 01:24

Yaml extension for Eigen::Matrix and std::unordered_map #175

Yaml extension for Eigen::Matrix and std::unordered_map #175

Conversation

gilwoolee commented Apr 16, 2017

mkoval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslee02 commented Apr 17, 2017

Choose a reason for hiding this comment

coveralls commented Apr 19, 2017

mkoval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psigen commented Apr 21, 2017 • edited Loading

jslee02 commented Apr 21, 2017

jslee02 commented Apr 21, 2017 • edited Loading

coveralls commented Apr 21, 2017

coveralls commented Apr 21, 2017

coveralls commented Apr 22, 2017

mkoval commented Apr 22, 2017 • edited Loading

jslee02 commented Apr 22, 2017

coveralls commented Apr 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 23, 2017

coveralls commented Apr 23, 2017

coveralls commented Apr 23, 2017

psigen commented Apr 21, 2017 •

edited

Loading

jslee02 commented Apr 21, 2017 •

edited

Loading

mkoval commented Apr 22, 2017 •

edited

Loading