AutoRally / autorally

Software for the AutoRally platform
http://autorally.github.io
727 stars 228 forks source link

Unused parameter `state` passed in `enforceConstraints` function of `template NeuralNetModel` #65

Closed mithi closed 4 years ago

mithi commented 6 years ago

In autorally/autorally_control/include/autorally_control/path_integral/neural_net_model.cut

Lines 245 - 253, the MatrixXf &state is passed as a parameter to a function, but it's not used... How come? Thank you!

template<int S_DIM, int C_DIM, int K_DIM, int... layer_args>
void NeuralNetModel<S_DIM, C_DIM, K_DIM, layer_args...>::enforceConstraints(Eigen::MatrixXf &state, Eigen::MatrixXf &control)
{
  int i;
  for (i = 0; i < CONTROL_DIM; i++){
    if (control(i) < control_rngs_[i].x){
      control(i) = control_rngs_[i].x;
    }
    else if (control(i) > control_rngs_[i].y){
      control(i) = control_rngs_[i].y;
    }
  }
}
gradyrw commented 6 years ago

It's plausible that someone could write a new dynamics class which enforces both state and control constraints, so we wanted to accommodate for that case. This way, if someone does implement a dynamics class which enforces some state constraints, they don't need to modify any of the other path integral code.

mithi commented 6 years ago

@gradyrw

Thanks for clarifying.. I have this last two question before I close this issue:

In run_control_loop() From line 86 to 88 of: https://github.com/AutoRally/autorally/blob/kinetic-devel/autorally_control/include/autorally_control/path_integral/run_control_loop.cuh

    u = controller.computeControl(state); //Compute the control
    controller.model_->enforceConstraints(state, u);
    controller.model_->updateState(state, u); //Update the state using motion model.

But when you look at updateState() it also uses enforce constraints inside.. May I ask why this is necessary? updateState()

I want to clarify, is the current control parameter passed by reference u modified inside updateState() ?

In updateState() from: line 245 - 253 of: https://github.com/AutoRally/autorally/blob/kinetic-devel/autorally_control/include/autorally_control/path_integral/neural_net_model.cut

template<int S_DIM, int C_DIM, int K_DIM, int... layer_args>
void NeuralNetModel<S_DIM, C_DIM, K_DIM, layer_args...>::updateState(Eigen::MatrixXf &state, Eigen::MatrixXf &control)
{
  enforceConstraints(state, control);
  computeKinematics(state);
  computeDynamics(state, control);
  state += state_der_*dt_;
  state_der_ *= 0;
}
gradyrw commented 6 years ago

Yes, it looks like the enforceConstraints call before calling updateState is actually redundant in this case. This may be changed in the future, but for now, it is not doing any harm.

JasonGibson274 commented 4 years ago

Closing since resolved