Error when loading a persistent parameter of type array<double>

[sukhrajklair]

@fujitatomoya I appreciate the work you've done here.

I ran into an issue when saving and loading an array of doubles. Problem is that the YAML emitter automatically converts the double values whose decimal part is 0 (ex. 1.0) to int. For example, if I set a parameter {1.0, 1.1}, the first number gets saved as 1 in the yaml file. When the parameter_server loads the yaml file, the rcl_yaml_param_parser throws an error saying Failed to parse parameters from custom yaml file '<file-path': Sequence should be of same type. Value type 'integer' do not belong at line_num 5, at ./src/parse.c:337.

The rcl_yaml_param_parser checks that when loading a yaml node that is a sequence, each value of that sequence should have the same type as the sequence itself.
https://github.com/ros2/rcl/blob/41b83c8ebfe8344a80c0093083b7abdfded79337/rcl_yaml_param_parser/src/parse.c#L325

I have tried to set tag of the Sequence Node to "!!float" in the StoreYamlFile() function but that doesn't help.

I believe this can be solved by forcing the YAML emitter to write decimal numbers as decimals even if the decimal part is 0. I couldn't figure out how to do it.

[fujitatomoya]

@sukhrajklair thanks for creating issue. this obviously sounds the unexpected behavior, i will take a look when i can find some spare time. in the meantime, could you provide reproducible parameter yaml to load the server or procedure? that would be helpful.

[sukhrajklair]

@fujitatomoya
Here are the steps to reproduce the issue:

1. launch parameter server
   ros2 launch parameter_server parameter_server.launch.py
2. set a parameter containing array of doubles with at least one double value ending in .0
   ros2 param set /parameter_server persistent.d_array '[1.0,1.1]'
3. Kill parameter_server and relaunch. At this point, it shows the following error:
   Catch exception: Failed to parse parameters from custom yaml file '/tmp/parameter_server.yaml': Sequence should be of same type. Value type 'double' do not belong at line_num 5

[fujitatomoya]

@sukhrajklair thanks, i will look into it.

[fujitatomoya]

I believe this can be solved by forcing the YAML emitter to write decimal numbers as decimals even if the decimal part is 0. I couldn't figure out how to do it.

yeah, this is not the problem for either ROS 2 nor parameter server, after pushing the double value to YAML::Node from rclcpp::Parameter, it is already gone wrong [1, 1.1000000000000001]. this hurts us when reloading the saved file to the parameter server, since it deduces 1 as integer, then eventually fails to load the parameter file.

see jbeder/yaml-cpp#1016

[fujitatomoya]

this is obviously yaml-cpp fix required. but if the fix does not backport to v0.7.0 (ubuntu 22.04), we might end up having the patched yaml-cpp in this repository. 1st, i can try the fix to yaml-cpp anyway.

note, this should be described as known issue with possible work-around for user application.

[sukhrajklair]

Thanks for looking into it. I am probably going to work around this by saving the values as a string and then parsing them.

I agree, yaml-cpp needs a fix. However, I don't see the need to check/throw an error when an int is provided where a double is expected by the rcl_yamal_param_parser. Why not just cast the int as a double? Am I missing something?

[fujitatomoya]

I am probably going to work around this by saving the values as a string and then parsing them.

yeah i was thinking the same work-around just like this, which is not really helpful for the application since they need to covert the parameters during read/write. but doable...

I agree, yaml-cpp needs a fix.

a few of consideration.

• fix yaml-cpp and backport right away. (best scenario)
• integrate fixed yaml-cpp with https://github.com/fujitatomoya/ros2_persist_parameter_server (not great for maintenance but full control by us)
• change the backend storage into lightweight database such as https://github.com/google/leveldb (opt-in option, because we lose the ROS 2 parameter file compatibility.)

However, I don't see the need to check/throw an error when an int is provided where a double is expected by the rcl_yamal_param_parser. Why not just cast the int as a double? Am I missing something?

that is very good question, yes and no.

as far as i see current implementation (i am not the one who implemented this 😅 ), in sequence, 1st value type prevails. saing

• [1, 1.1000000000000001] -> it deduces sequence type is int, because 1st one is the int. and double cannot be handled since we end up the accuracy. and it is likely that this is not user's intention, so fail to parse to notify the user application. i think that is correct behavior.

on the other hand,

• [1.1000000000000001, 1] -> it deduces sequence type is double, because 1st one is the double. in this case, we could continue parsing since we are not losing any accuracy here. and user might think this can be parsed as double.

i think we can make the above change, but downside is not consistent behavior for user application. for application view point, it sometimes can be parsed, but sometime cannot. to keep the consistent behavior, i say in the sequence 1st one prevails and the following values must be the same type of that. (and i think that is aligned with https://yaml.org/spec/1.2.2/)

[sukhrajklair]

I am probably going to work around this by saving the values as a string and then parsing them.

yeah i was thinking the same work-around just like this, which is not really helpful for the application since they need to covert the parameters during read/write. but doable...

I agree, yaml-cpp needs a fix.

a few of consideration.

• fix yaml-cpp and backport right away. (best scenario)

looks like the fix mentioned in jbeder/yaml-cpp#1016 was never added. I think this will be helpful to other people as well.

• integrate fixed yaml-cpp with https://github.com/fujitatomoya/ros2_persist_parameter_server (not great for maintenance but full control by us)

let me know if I can help.

• change the backend storage into lightweight database such as https://github.com/google/leveldb (opt-in option, because we lose the ROS 2 parameter file compatibility.)

this could have other uses such as centralized configuration management for a fleet of robots.

However, I don't see the need to check/throw an error when an int is provided where a double is expected by the rcl_yamal_param_parser. Why not just cast the int as a double? Am I missing something?

that is very good question, yes and no.

as far as i see current implementation (i am not the one who implemented this 😅 ), in sequence, 1st value type prevails. saing

• [1, 1.1000000000000001] -> it deduces sequence type is int, because 1st one is the int. and double cannot be handled since we end up the accuracy. and it is likely that this is not user's intention, so fail to parse to notify the user application. i think that is correct behavior.

on the other hand,

• [1.1000000000000001, 1] -> it deduces sequence type is double, because 1st one is the double. in this case, we could continue parsing since we are not losing any accuracy here. and user might think this can be parsed as double.

i think we can make the above change, but downside is not consistent behavior for user application. for application view point, it sometimes can be parsed, but sometime cannot. to keep the consistent behavior, i say in the sequence 1st one prevails and the following values must be the same type of that. (and i think that is aligned with https://yaml.org/spec/1.2.2/)

That makes sense now. thanks for the explanation :)

[fujitatomoya]

actually i tried the following patch, which does not fix the problem.

diff --git a/include/yaml-cpp/node/convert.h b/include/yaml-cpp/node/convert.h
index 596898d..502deac 100644
--- a/include/yaml-cpp/node/convert.h
+++ b/include/yaml-cpp/node/convert.h
@@ -9,6 +9,7 @@

 #include <array>
 #include <cmath>
+#include <iomanip>
 #include <limits>
 #include <list>
 #include <map>
@@ -109,7 +110,7 @@ inner_encode(const T& rhs, std::stringstream& stream){
       stream << ".inf";
     }
   } else {
-    stream << rhs;
+    stream << std::setprecision(12) << std::fixed << rhs;
   }
 }

[fujitatomoya]

The problem stays in the master: jbeder/yaml-cpp@eaf7205

[fujitatomoya]

TODO: I guess at least we need to add in documentation as known issue.