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

Created utility function for adding many priors simultaneously #125

Closed
wants to merge 3 commits into from

Conversation

scharalambous3
Copy link
Collaborator

Created a utility function in initialize_solution_utils that accepts multiple keys and state terms to resolve copy/pasta in the examples

Checked the generated trajectories of the affected examples with csvdiff ✓
Unit test ✓

Cc: @dellaert (resolves review comment in #100) @yetongumich @gchenfc

graph.addPrior(internal::JointVelKey(j0_id, 0), X_i[1], dynamics_model);
graph.addPrior(internal::JointAngleKey(j1_id, 0), X_i[3], dynamics_model);
graph.addPrior(internal::JointVelKey(j1_id, 0), X_i[4], dynamics_model);
graph = FactorGraphConditions(
Copy link
Member

Choose a reason for hiding this comment

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

Several comments which you can generalize to the entire PR:

  1. this won't work as you're not adding but replacing the graph
  2. It won't generalize to cases where we have > 2 joints
  3. I propose renaming to AddSoftKinematicsConstraint
  4. I propose to do the key business inside the function so the four lines become:
AddSoftKinematicsConstraint(&graph, dynamics_model, {X_i[0], X_i[1]}, j0_id); // angle and vel for joint 0
AddSoftKinematicsConstraint(&graph, dynamics_model. {X_i[2], X_i[3]}, j1_id); // angle and vel for joint 1

Note t should the last argument and by default 0, as in values.h

Copy link
Member

Choose a reason for hiding this comment

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

Is the word "Soft" appropriate since the caller can pass in a Constrained noise model if they choose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks Frank for the review. I will make changes accordingly. Please correct me if I'm wrong but I will have to add the key function as an input parameter, too
Gerry , my understanding is that "Soft" is appropriate because the conditions are only enforced as soft constraints

Copy link
Member

Choose a reason for hiding this comment

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

No, the keys are constructed inside from the joint ID and the time "t".

Copy link
Member

Choose a reason for hiding this comment

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

@gchenfc you are right. But what word to use if not "SoftConstraint" ? Constraint implies hard. Factor is too generic.

Copy link
Member

Choose a reason for hiding this comment

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

Then again, we do talk about soft and hard constraints so maybe the best naming, unless something better comes along, is just omitting the "Soft"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd hold off working on this until the spider-walking branch is merged because it may not be necessary

@scharalambous3
Copy link
Collaborator Author

Closing this because ObjectiveFactors.h in #148 fulfilled this requirement!

@varunagrawal varunagrawal deleted the fix/conditions branch April 23, 2021 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants