test(state-representation): add utility tests for Cartesian/JointState

[bpapaspyros]

Description

• Add tests for new Cartesian and Joint state utilities #201

This PR solves the issue by updating the corresponding tests to verify the bindings in python and the functionality in C++.

Review guidelines

Estimated Time of Review: 5 minutes

Checklist before merging:

• Confirm that the relevant changelog(s) are up-to-date in case of any user-facing changes

[domire8]

Looks good, could you add

• multiply state variable for joint states
• add an assertion that it fails if you provide a vector of wrong size

[bpapaspyros]

Looks good, could you add

• multiply state variable for joint states

Adding soon.

• add an assertion that it fails if you provide a vector of wrong size

While writing the test for this I realized something somehting else that we might need to address in a follow up PR, which is a bit problematic when inputs are eigen vectors (C++).

For example:

  auto state = CartesianState();
  auto new_incompatible_values = Eigen::VectorXd(4);
  new_incompatible_values << 1.0, 2.0, 3.0, 4.0;
  EXPECT_THROW(state.set_state_variable(new_incompatible_values, state_variable_type), exceptions::IncompatibleSizeException);

For the new methods that are exposed this is correct (as in, it throws our custom exception).

However:

  auto state = CartesianState();
  auto new_incompatible_values = Eigen::VectorXd(4);
  new_incompatible_values << 1.0, 2.0, 3.0, 4.0;
  EXPECT_THROW(state.set_position(new_incompatible_values), exceptions::IncompatibleSizeException);

is problematic, because the signature for set_position is:

void CartesianState::set_position(const Eigen::Vector3d& position) {
  this->set_state_variable(position, CartesianStateVariable::POSITION);
}

which means that C++ will implicitly cast the VectorXd down to Vector3d which will cause an exception much earlier than our internal check in set_state_variable. This might not be explicitly our responsibility, but I feel like we should mark this function (and perhaps others) as explicit and prevent this kind of uses (@eeberhard adding you mostly for this point and whether you think a follow up PR would make sense)

[domire8]

Thanks for writing this up. Your suggestion was to add [[explicit]] is that correct? That would have as effect that the compilation would already fail?

[bpapaspyros]

Thanks for writing this up. Your suggestion was to add [[explicit]] is that correct? That would have as effect that the compilation would already fail?

Well actually my bad. We can not use explicit on a function (just constructors, destructors and conversion operators), which means typesafety here would require sfinae, C++20, or deleting overloads. I don't like the first and last at the current point, and C++20 is also a big change.

I think for now we can open a small backlog item, but we might just be ok with delegating responsibility downwards for this.

[domire8]

I think for now we can open a small backlog item, but we might just be ok with delegating responsibility downwards for this.

Yeah these changes seem to be a bit too big for now, a backlog item with no immediate action taken seems fair for now

[bpapaspyros]

Thanks for writing this up. Your suggestion was to add [[explicit]] is that correct? That would have as effect that the compilation would already fail?

Well actually my bad. We can not use explicit on a function (just constructors, destructors and conversion operators), which means typesafety here would require sfinae, C++20, or deleting overloads. I don't like the first and last at the current point, and C++20 is also a big change.

I think for now we can open a small backlog item, but we might just be ok with delegating responsibility downwards for this.

For reference we could do something like:

SFINAE

template <typename T>
typename std::enable_if<std::is_same<T, Eigen::Vector3d>::value>::type
void CartesianState::set_position(const T& position) {
  this->set_state_variable(position, CartesianStateVariable::POSITION);
}

or C++20:

template <typename T>
concept IsEigenVec3 = std::same_as<T, Eigen::Vector3d>;

void CartesianState::set_position(const IsEigenVec3& position) {
  this->set_state_variable(position, CartesianStateVariable::POSITION);
}

seems a bit too much boilerplate.

Perhaps the simplest is to just:

void CartesianState::set_position(const Eigen::VectorXd& position) {
  this->set_state_variable(position, CartesianStateVariable::POSITION);
}

and let the set_state_variable throw the exception

[domire8]

The changes of this PR seem good to me, thanks!

[domire8]

Perhaps the simplest is to just:

Sure but that is even less correct to me IMO because the function signature tells the user that it requires a Vector3d which is the only logical choice for position

[bpapaspyros]

Perhaps the simplest is to just:

Sure but that is even less correct to me IMO because the function signature tells the user that it requires a Vector3d which is the only logical choice for position

I agree, yes.