Let integer values be assigned to double parameters #1024

jacobperron · 2020-03-12T22:04:12Z

Fixes #979.

When constructing integer ParameterValue objects, also set the double member.
When requesting a double value for ParameterValue objects of type integer, do not throw an exception.

Fixes #979. When constructing integer ParameterValue objects, also set the double member. When requesting a double value for ParameterValue objects of type integer, do not throw an exception. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

wjwwood

After talking off-line, @jacobperron told me that the driving use case is auto declare_parameter(name, default, ...), where the return type of that function dependent on the deduced type of default. In that case a copy must be returned anyways, and so it's an ideal place to convert from int to double (or more specifically vector<int> to vector<double>) if we want to do that for users.

Also, we could consider changing the ParameterValue::get<int64_t/double/bool> so that it returns a value rather than a reference. I don't remember if we tried that before, but I know @clalancette brought it up at some point too, that return const bool & (for example) is kind of silly and not recommended. That would allow us to implicitly convert to double from int when return the value, which we cannot easily do now because we return a const reference atm. That's why @jacobperron chose to store it twice, so we could return different references based on what the user asked for. This change would not, however, address the case where the user asked for vector<double> but the parameter is being stored as a vector<int>, but as I said above that could be handled in the auto declare_parameter signature because that's unconditionally returning a copy.

I would not advocate for changing the ParameterValue::get<vector<int/double>/string> to return a copy rather than a const reference. Because we'd like people to be able to access their parameter values without memory allocations. However, we could consider adding a get_copy for these, or even change get to return a copy but add a get_ref for these types, and people who care about the memory allocations could explicitly use get_ref.

wjwwood · 2020-03-13T00:00:56Z

rclcpp/src/rclcpp/parameter_value.cpp

@@ -189,12 +193,16 @@ ParameterValue::ParameterValue(const std::vector<bool> & bool_array_value)
 ParameterValue::ParameterValue(const std::vector<int> & int_array_value)
 {
  value_.integer_array_value.assign(int_array_value.cbegin(), int_array_value.cend());
+  // Allow integer arrays to be cast to double arrays
+  value_.double_array_value.assign(int_array_value.cbegin(), int_array_value.cend());


I'm not a fan of this. For primitive types it is not as bad (though I still don't like the idea that the value has two values stored), but for the array I think there's a chance that could be lots of overhead.

clalancette · 2020-03-16T15:19:56Z

I would not advocate for changing the ParameterValue::get<vector<int/double>/string> to return a copy rather than a const reference. Because we'd like people to be able to access their parameter values without memory allocations. However, we could consider adding a get_copy for these, or even change get to return a copy but add a get_ref for these types, and people who care about the memory allocations could explicitly use get_ref.

I'm generally fine with a get/get_copy split (or however we want to split them), though I am concerned that their semantics will be different. That is, I don't want it to be the case that get_copy can take an integer for a double parameter while get cannot. If we can make the semantics exactly the same for both, then I'd be all for that split.

wjwwood · 2020-03-17T00:57:01Z

If we can make the semantics exactly the same for both, then I'd be all for that split.

The only reason to have both is so the automatic conversion can happen... Otherwise you could just have a const and non-const method for each called get(), i.e. overloading.

The idea was, @jacobperron correct me if I'm wrong, to have get() never convert (because it has to store both versions because it cannot convert in place because that would return a copy to a temporary) and have get_copy() where it can convert (because it is returning a value not a reference).

clalancette · 2020-03-17T14:50:18Z

The only reason to have both is so the automatic conversion can happen... Otherwise you could just have a const and non-const method for each called get(), i.e. overloading.

The idea was, @jacobperron correct me if I'm wrong, to have get() never convert (because it has to store both versions because it cannot convert in place because that would return a copy to a temporary) and have get_copy() where it can convert (because it is returning a value not a reference).

Personally, I think that is a worse place to be in from an API point of view.

It means you'll end up with some downstream API users using get(), some using get_copy(), and inconsistent conversion semantics.

As an alternative, I wonder if it would be better to just improve the error messages that people get when we can't do conversions. Right now it's pretty unclear both why the conversion can't happen, and which parameter is causing the problem. Adding error messages explaining both of those things would go a long way towards improving the user experience.

wjwwood · 2020-03-17T18:25:45Z

Personally, I see the frustration from the user perspective, but I also think it's not our place to do this conversion for them, simply because std::variant doesn't do this for you:

std::variant<int, double> foo{1};
std::get<double>(f);  // exception here

Whereas in the proposed example of ours:

ParameterValue p(1);
p.get<double>();  // exception right now, no exception with this pr

clalancette · 2020-03-17T18:43:17Z

simply because std::variant doesn't do this for you:

Just to clarify: would you say that you agree with my proposal from the previous comment? That is, we should not take this PR and instead improve the error messages that happen when automatic conversion fails?

wjwwood · 2020-03-17T19:35:21Z

Sort of I guess.

I would like to solve the issue for users since it is annoying, but on the other hand its very tricky to do correctly and is part of the reason why std::variant doesn't do it for you...

@jacobperron's original pr addresses your concern @clalancette, but it requires that you store all integers as doubles as well. And if we ever updated to use std::variant instead then we wouldn't be able to continue doing that.

This reverts commit bd38110.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2020-03-17T21:03:49Z

I agree that having separate APIs for getting copies and references is not a great solution. What if we just do the conversion for the primitive (i.e int -> double), but not for the array variant? And then we can provide a good error message for the array case.

I think we can accomplish this by returning primitives by value (always), and returning arrays by reference. I think the overhead for returning primitives by value is negligible.

Fixes #979 Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2020-03-17T21:21:07Z

@wjwwood @clalancette I've reverted the original commit (b65f95e) and added two new changes:

Return primitive parameter values by value (9d17592)
Cast integer parameters to double if requested (b67522e)

If users try to get a double array from an integer array they will still get a ParameterTypeException. I'll look into improving the error message for users in a separate PR.

wjwwood

This works for me, but I'm interested to hear from @clalancette again.

wjwwood · 2020-03-18T03:05:11Z

rclcpp/include/rclcpp/parameter_value.hpp

  get() const
  {
-    if (value_.type != rcl_interfaces::msg::ParameterType::PARAMETER_DOUBLE) {
-      throw ParameterTypeException(ParameterType::PARAMETER_DOUBLE, get_type());
+    // Allow integer values to be cast to double


We should really document this somewhere, maybe in the high level get/declare_parameter() APIs, or maybe just right here.

clalancette · 2020-03-19T13:16:31Z

This works for me, but I'm interested to hear from @clalancette again.

I hate to be the naysayer, but I still think that having it different in different contexts is a worse API. That is, the fact that it is different between arrays and primitives just feels really ugly to me. It's also kicking the can down the road; we're going to get exactly the same complaint about arrays before too long.

I have a longer post to write here to try and come up with yet another solution. I'll get to that later today, but I wanted to get my thoughts in now since this has been sitting for a couple of days.

clalancette · 2020-04-08T14:59:35Z

Sorry this totally fell off of my radar. Here are some further thoughts.

The ultimate goal here is to make things easier for users, so that they don't have to write 0.0 and instead can just write 0. The first question to answer, which I don't think we've examined enough, is whether we actually want to do this. I'm personally a fan of being very explicit, so I'd vote for not doing this, and instead improve the error messages so that it is explicit about what is failing. I think we've done this somewhat elsewhere; if need be, we can press more on that front. Maybe it is worthwhile doing a discourse post or similar to get other ideas here?

(I may have some ideas on different ways we could accomplish it if we decide to go forward, but I think we really need to answer that question first)

jacobperron · 2020-04-08T16:29:02Z

I'm okay with forcing users to be explicit. This change was driven by interactions with users that thought that this implicit cast should happen for them, and I think we'll get similar feedback on Discourse, but we should make a post to confirm.

If we do decide to go forward with this change, it sounds like it will need some significant refactoring so it's not going to make it for Foxy. We can table it until after Foxy.

jacobperron · 2020-04-08T16:29:26Z

Here's an improvement on the error message: #1027

jacobperron · 2021-03-25T22:15:32Z

Closing this since I don't have any intention on completing it.

* Refactors the common parts of play_until and play_duration tests. - Migrates test fixtures of play with duration and until a timestamp options to a common header file. - Moves test_play_for and PlayFor to use duration instead. Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com> * Update rosbag2_transport/test/rosbag2_transport/test_play_until.cpp Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com> * Update rosbag2_transport/test/rosbag2_transport/test_play_until.cpp Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com> * Fixes formatting. Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com> Co-authored-by: Michael Orlov <morlovmr@gmail.com>

jacobperron added the enhancement New feature or request label Mar 12, 2020

jacobperron self-assigned this Mar 12, 2020

jacobperron requested review from clalancette, mabelzhang and wjwwood March 12, 2020 22:05

wjwwood reviewed Mar 13, 2020

View reviewed changes

jacobperron added 2 commits March 17, 2020 13:58

Revert "Let integer values be assigned to double parameters"

b65f95e

This reverts commit bd38110.

Get primitive parameters by value

9d17592

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Implicitly cast integer parameter values to double

b67522e

Fixes #979 Signed-off-by: Jacob Perron <jacob@openrobotics.org>

wjwwood approved these changes Mar 18, 2020

View reviewed changes

jacobperron mentioned this pull request Mar 18, 2020

Implicitly cast integer values for double parameters #979

Closed

jacobperron added this to In progress in Foxy API Freeze Apr 7, 2020

jacobperron removed this from In progress in Foxy API Freeze Apr 10, 2020

jacobperron added this to In progress in Galactic via automation Apr 10, 2020

jacobperron closed this Mar 25, 2021

jacobperron removed this from In progress in Galactic Mar 25, 2021

jacobperron deleted the jacob/int_as_double_param branch March 25, 2021 22:16

henningkayser mentioned this pull request Apr 19, 2021

Servo: Ensure yaml param types are correct moveit/moveit2#427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let integer values be assigned to double parameters #1024

Let integer values be assigned to double parameters #1024

jacobperron commented Mar 12, 2020

wjwwood left a comment

wjwwood Mar 13, 2020

clalancette commented Mar 16, 2020

wjwwood commented Mar 17, 2020

clalancette commented Mar 17, 2020

wjwwood commented Mar 17, 2020

clalancette commented Mar 17, 2020

wjwwood commented Mar 17, 2020

jacobperron commented Mar 17, 2020

jacobperron commented Mar 17, 2020 •

edited

Loading

wjwwood left a comment

wjwwood Mar 18, 2020

clalancette commented Mar 19, 2020

clalancette commented Apr 8, 2020

jacobperron commented Apr 8, 2020

jacobperron commented Apr 8, 2020

jacobperron commented Mar 25, 2021

Let integer values be assigned to double parameters #1024

Let integer values be assigned to double parameters #1024

Conversation

jacobperron commented Mar 12, 2020

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood Mar 13, 2020

Choose a reason for hiding this comment

clalancette commented Mar 16, 2020

wjwwood commented Mar 17, 2020

clalancette commented Mar 17, 2020

wjwwood commented Mar 17, 2020

clalancette commented Mar 17, 2020

wjwwood commented Mar 17, 2020

jacobperron commented Mar 17, 2020

jacobperron commented Mar 17, 2020 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood Mar 18, 2020

Choose a reason for hiding this comment

clalancette commented Mar 19, 2020

clalancette commented Apr 8, 2020

jacobperron commented Apr 8, 2020

jacobperron commented Apr 8, 2020

jacobperron commented Mar 25, 2021

jacobperron commented Mar 17, 2020 •

edited

Loading