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

Gpm null space dimension print #126

Closed

Conversation

bbrito
Copy link
Contributor

@bbrito bbrito commented Jan 12, 2017

Solution for issue #125.

Copy link
Contributor

@fmessmer fmessmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbrito bbrito force-pushed the gpm_null_space_dimension_print branch from 4cccec9 to 9f4c64e Compare January 12, 2017 16:31
@bbrito bbrito force-pushed the gpm_null_space_dimension_print branch from 9f4c64e to 58be1d3 Compare January 12, 2017 16:34
Copy link
Contributor

@fmessmer fmessmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requesting changes

@@ -52,19 +52,29 @@ Eigen::MatrixXd GradientProjectionMethodSolver::solve(const Vector6d_t& in_cart_

Eigen::MatrixXd homogeneous_solution = Eigen::MatrixXd::Zero(particular_solution.rows(), particular_solution.cols());
KDL::JntArrayVel predict_jnts_vel(joint_states.current_q_.rows());
Eigen::FullPivLU<Eigen::MatrixXd> lu_decomp(projector);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance question:
I think it is quite expensive to perform a FullPivLU to determine rank
Could we not just check whether the determinant of a PartialPivLU is 0.0?

Or even easier:
compare projector with a Zero-Matrix using isApprox

@@ -106,12 +106,23 @@ Eigen::MatrixXd StackOfTasksSolver::solve(const Vector6d_t& in_cart_velocities,
Eigen::MatrixXd J_temp = J_task * projector_i;
Eigen::VectorXd v_task = it->task_;
Eigen::MatrixXd J_temp_inv = pinv_calc_.calculate(J_temp);
Eigen::FullPivLU<Eigen::MatrixXd> lu_decomp(J_temp_inv);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you evaluate J_temp_inv and not projector_i?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because J_temp_inv is going to project the ith task velocities. If it has rank zero is not adding any velocity for that task.

qdots_out.col(0) = q_i;
ROS_WARN("Null space projection matrix is null. It couldn't satisfy all constraints");
return qdots_out;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we cannot quit here as projector_i = projector_i - J_temp_inv * J_temp; so even if some projector_i are Zero, the overall projector_i not necessarily is Zero as it is a sum not a multiplication.

Just a warning that the projector of the current task is Zero but continue computation


qdots_out.col(0) = q_i + projector_i * sum_of_gradient;
return qdots_out;
Eigen::FullPivLU<Eigen::MatrixXd> lu_decomp(projector_i);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same performance comment as above

return qdots_out;
}
qdots_out.col(0) = q_i + projector_i * sum_of_gradient; //WHY???
return qdots_out;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why //WHY???

identation

@@ -64,8 +64,9 @@ Eigen::MatrixXd TaskPrioritySolver::solve(const Vector6d_t& in_cart_velocities,
predict_jnts_vel.q(i) = particular_solution(i, 0) * cycle + joint_states.current_q_(i);
predict_jnts_vel.qdot(i) = particular_solution(i, 0);
}
Eigen::FullPivLU<Eigen::MatrixXd> lu_decomp(projector);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same performance question


if (this->constraints_.size() > 0)
if ((this->constraints_.size() > 0) && (lu_decomp.rank() != 0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put the check into a new if?
Like this:

if (this->constraints_.size() > 0)
{
    if(lu_decomp.rank() != 0)
    {

because than it's more clear was has happened, i.e. whether the first or the second condition is false

@fmessmer
Copy link
Contributor

fmessmer commented Jan 14, 2017

I played around with this a bit but couldn't enforce a situation where the projection matrix actually becomes Zero, i.e. rank 0 or determinant 0...maybe we have to discuss this a bit more...:wink:

@fmessmer
Copy link
Contributor

fmessmer commented Jan 19, 2017

See the last two commits on my branch where I tested various methods for testing empty Nullspace...
It seems that isApprox is not suited to determine empty Nullspace

For matrices, the comparison is done using the Hilbert-Schmidt norm (aka Frobenius norm L2 norm).
Because of the multiplicativeness of this comparison, one can't use this function to check whether *this is approximately equal to the zero matrix or vector. Indeed, isApprox(zero) returns false unless *this itself is exactly the zero matrix or vector. If you want to test whether *this is zero, use internal::isMuchSmallerThan(const RealScalar&, RealScalar) instead.

Also, I did quite some indentation fixing

@fmessmer
Copy link
Contributor

fmessmer commented Apr 7, 2017

@ipa-bfb
should we close this unmerged as it does not really help in debugging anyway...
I'll add a pointer to this PR to #152 in order to find it, in case we need it again

@fmessmer
Copy link
Contributor

@ipa-bfb FYI

as discussed in https://github.com/ipa320/cob_control/pull/126#issuecomment-292532450 and because a pointer is added in https://github.com/ipa320/cob_control/issues/152#issuecomment-292532872, I'll close it unmerged...has a merge-conflict anyway

@fmessmer fmessmer closed this Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants