jrl-umi3218 / mc_rtc

mc_rtc is an interface for simulated and real robotic systems suitable for real-time control
BSD 2-Clause "Simplified" License
122 stars 37 forks source link

yes/no text is interpreted as boolean in yaml configuration #212

Closed mmurooka closed 2 years ago

mmurooka commented 2 years ago

Yes/no text is always interpreted as boolean when making mc_rtc::Configuration instance from yaml file. This is inconvenient when dealing with functions that receive yes / no strings like this: https://coin-or.github.io/Ipopt/OPTIONS.html#OPT_derivative_test_print_all

Yaml configuration file:

NlpSolver:
  derivative_test_print_all: yes

C++ source to dump the mc_rtc::Configuration instance:

if (config.has("NlpSolver")) {
  mc_rtc::log::info("{}", config("NlpSolver").dump(true));
}

Output:

{
    "derivative_test_print_all": true,
}

Even if replacing yes in the yaml file to 'yes' or "yes", the result was same. If the yaml parser can distinguish between yes and "yes", I think the latter should not be converted to boolean.

The limited workaround I'm doing now is

  std::string value;
  try {
    value = static_cast<std::string>(config(key));
  }
  catch (mc_rtc::Configuration::Exception& ex) {
    if (static_cast<bool>(config(key))) {
      value = "yes";
    } else {
      value = "no";
    }
    ex.silence();
  }
  SetOption(key, value);
gergondet commented 2 years ago

Hi @mmurooka

Thanks for bringing this up. I noticed and fixed a similar conversion happening for the y/n values in #208

The problem appears because we convert the YAML to mc_rtc::Configuration (and thus ultimately JSON) internally and to do so we check what the YAML value can be converted too. Since YAML 1.1 accepts y/n/yes/no/true/false as valid boolean representations we then convert those to bool. To change this behavior we have to explicitly check what the YAML parser accepts as a boolean.

I'm ok with removing the yes/no conversion. This can be done by simply removing the check here: https://github.com/jrl-umi3218/mc_rtc/blob/master/src/mc_rtc/internals/yaml.h#L42 and instead always check the value of scalar. I'll make a PR for this later but you can try it locally.

mmurooka commented 2 years ago

@gergondet thank you for your explanation and Pull Request!