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

Optional def arg #565

Merged
merged 3 commits into from
May 13, 2019
Merged

Conversation

christian-rauch
Copy link
Collaborator

@christian-rauch christian-rauch commented May 12, 2019

As discussed in #561, this PR changes the initialiser code generator to require default arguments for Optional paramters.

It already triggered for the JointPose task map initiliasier (Optional parameter 'JointRef' requires a default argument!) which is a Eigen Matrix type and would load garbage values if not set by the user.

What would be a good default value for these:

class JointPose
extend <exotica_core/task_map>
Optional Eigen::VectorXd JointRef;
Optional Eigen::VectorXd JointMap;

or should they be Required?

Edit: I already made them Required to let the CI pass.

@wxmerkt
Copy link
Collaborator

wxmerkt commented May 12, 2019

I don't think they have triggered "garbage" values for JointPose since it was correctly handled in the Initialize call (and we frequently use JointPose as a default-cost with the empty optional parameters, defaulting to "all joints squared/from zero"):

if (parameters_.JointMap.rows() > 0)
{
joint_map_.resize(parameters_.JointMap.rows());
for (int i = 0; i < parameters_.JointMap.rows(); ++i)
{
joint_map_[i] = parameters_.JointMap(i);
}
}
else
{
joint_map_.resize(N_);
for (int i = 0; i < N_; ++i)
{
joint_map_[i] = i;
}
}
if (parameters_.JointRef.rows() > 0)
{
joint_ref_ = parameters_.JointRef;
if (joint_ref_.rows() != joint_map_.size()) ThrowNamed("Invalid joint reference size! Expecting " << joint_map_.size() << " but received " << joint_ref_.rows());
}
else
{
joint_ref_ = Eigen::VectorXd::Zero(joint_map_.size());
}

I.e. since they are correctly handled in the implementation they neither need a default value nor need to be required. For double/int/other standard types, this is, of course, different - but for Eigen-types we usually handle it well.

@VladimirIvan
Copy link
Collaborator

These should be optional and set to empty vectors.

@wxmerkt
Copy link
Collaborator

wxmerkt commented May 12, 2019

I think I found another, more narrow bug related to this and will update #561 with my findings shortly. It may be limited to fixed-size Eigen types. It appears to be limited to initialising manually from Python or without going through the XML-layer.

Copy link
Collaborator

@wxmerkt wxmerkt left a comment

Choose a reason for hiding this comment

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

Having dug into the issue with the minimal example you provided it seems that there are two distinct things at play:

(a) To require Optional arguments to have a default parameter - what you are proposing to fix in this PR. This should go ahead. Please revert and fix the change to JointPose though.

(b) The fact that Required arguments are not enforced if the initializer is passed manually via Python. If you create the same example in XML, it throws.

exotations/exotica_core_task_maps/init/joint_pose.in Outdated Show resolved Hide resolved
@christian-rauch
Copy link
Collaborator Author

This PR only addresses the issue that Optional parameters should have default arguments (a). It's not related to the bug that you can initialise Required parameters without a value via a Python dict (b).
Regarding the JointPose, I did not meant to say that garbage values will be loaded. For Eigen types, a class can always check of a dynamic sized matrix has been initialised. I just meant to say that if someone uses Eigen types like this without providing a default argument (or checking if it is initialised), it would provide garbage values.

Copy link
Collaborator

@wxmerkt wxmerkt left a comment

Choose a reason for hiding this comment

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

Thank you for the fix :-) Happy to merge once CI goes green.

@wxmerkt wxmerkt merged commit 83ae387 into ipab-slmc:master May 13, 2019
@christian-rauch christian-rauch deleted the optional_def_arg branch May 13, 2019 10:04
wxmerkt added a commit that referenced this pull request Jun 22, 2020
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.

3 participants