Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implicitly cast integer values for double parameters #979

Closed
jacobperron opened this issue Feb 4, 2020 · 8 comments
Closed

Implicitly cast integer values for double parameters #979

jacobperron opened this issue Feb 4, 2020 · 8 comments
Assignees
Labels
enhancement New feature or request

Comments

@jacobperron
Copy link
Member

Feature request

Feature description

If we try to pass an integer value to a double parameter from the command line or from a parameters YAML file we get a rclcpp::ParameterTypeException. For example, passing a parameter from the command line:

ros2 run foo_package foo_node --ros-args -p foo_arg:=1

results in the following error:

terminate called after throwing an instance of 'rclcpp::ParameterTypeException'
  what():  expected [double] got [integer]

and we can fix it by explicitly making our value a floating point number:

ros2 run foo_package foo_node --ros-args -p foo_arg:=1.0

But, it seems reasonable to me that if a user forgets to explicitly provide a floating point value that we should implicitly cast an integer to a float (as is done in many programming languages).

@ivanpauno
Copy link
Member

@jacobperron you were assigned to this from waffle meeting.
I think @clalancette and @mabelzhang were also interested in this.

@maxlein
Copy link

maxlein commented Feb 14, 2020

Can you add the affecting parameters name to the exception when you tackle this pls?

For example if you load a yaml file or a list of parameters in a launch file, it can be really annoying finding the wrong parameter.

@jacobperron jacobperron added this to To do in Foxy via automation Mar 12, 2020
jacobperron added a commit that referenced this issue Mar 12, 2020
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>
@jacobperron jacobperron moved this from To do to Review in progress in Foxy Mar 12, 2020
@jacobperron
Copy link
Member Author

Can you add the affecting parameters name to the exception when you tackle this pls?

I'm not sure the best way to add the parameter name, since the exception is coming from ParamterValue, which has no notion of the parameter name. We can probably catch the exception higher up and throw a different exception with the parameter name.

jacobperron added a commit that referenced this issue Mar 17, 2020
Fixes #979

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

See #1024 for a patch that allows integer values to be cast to double. This does not include arrays of integers being cast to arrays of double (for technical reasons).

Regarding improving the error message, two options come to mind:

  1. Catch the ParameterTypeException wherever rclcpp is calling ParameterValue::get and re-throw a new exception (InvalidParameterTypeException) that has a message including the parameter name.
    • Con: Users can still get the less informative ParameterTypeException if they call ParameterValue::get directly.
  2. Add a member to ParameterValue for the parameter name. Then this name can be used to improve the existing ParameterTypeException message.
    • Con: Having the name member in both Parameter and ParameterValue classes seems redundant.

I'm leaning towards (2), unless there's some unforeseen consequence to this approach.

@wjwwood Thoughts?

@jacobperron
Copy link
Member Author

jacobperron commented Mar 18, 2020

I looked into implementing both options. Option (2) certainly seems more intrusive as it requires touching all of the ParameterValue constructors for passing the parameter name and any code that is constructing ParameterValues. I'm starting to think that (1) might make more sense.

@jacobperron
Copy link
Member Author

See #1027 for an implementation of (2).

@jacobperron jacobperron removed this from Review in progress in Foxy Apr 27, 2020
@jacobperron jacobperron added this to To do in Galactic via automation Apr 27, 2020
@clalancette
Copy link
Contributor

Given that we closed #1024, we can close this as well. @jacobperron feel free to reopen if you think this is in error.

@clalancette clalancette removed this from To do in Galactic Mar 29, 2021
@jacobperron
Copy link
Member Author

I closed #1024 since there wasn't a clear path to a solution and I don't intend to spend more time working on it.
But maybe this is still something we want to tackle at some point in the future and worth leaving open; I'm not sure.

nnmm pushed a commit to ApexAI/rclcpp that referenced this issue Jul 9, 2022
…gth beyond length of string (ros2#979)

* Test that lexer never returns length longer than string

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

* Fix bug where lexer returned length longer than string

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

* Test that peeking 2 ahead never goes beyond NONE or EOF

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

* Stop peeking if the first lexeme is NONE or EOF

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants