Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add a first shell of the agimus_controller API #97
Add a first shell of the agimus_controller API #97
Changes from 28 commits
7a629d2
4397255
7fe2969
6b643cf
6c654f7
1490b3e
8e55069
8853e34
c8f2ab8
370bcb9
a21f0fb
2d91d3f
2b02a53
870a8f5
d24d4fe
ebdd0a6
10c0b65
66e6855
781becc
b0d6246
505c90a
ac00b8f
14c69f0
b851f4b
c5661be
c4ef2dc
0967dc9
a6a47c2
1126d21
748425d
b109535
637222c
462e498
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in crocoddyl there's no cost for only q or qdot so I guess we only have this instead :
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the weights are consistent with the trajectory point entries. I would say we either
state
instead ofconfiguration
andvelocity
or 2) keep them separate and merge in ocp to state
Also, I remember @MaximilienNaveau mentioning that state is not always [config, velocity], so option 1) might be misleading in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the second option how would we merge it in ocp to state ? the state will indeed not always be [config, velocity], but maybe it will still needs only one weight, that's something we have to check I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more information the better. If you have weights that you do not use: so be it.
But I don't believe so. In your case you should have nv+nv weights for x which means you have the same amount of weights than in the message. It might happen that a lot of weights are identical. But again this way you give more flexibility to the user with a small cost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ocp is responsible to recreate the weights matrix. And it knows everything: x, robot_model, costs etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you talking about the activation weights when creating the residuals ? in this case I agree there are nv+nv weights or maybe something else, but for the cost itself it's always a scalar right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well you can see the quadratic cost like so:
$$
alpha * X^T W X
$$
What the weights inside the message will give you is W as a diagonal matrix.
Should we consider adding the alpha as well?
I would believe that W is enough to weight the cost. If all diagonal members are equal you can actually factorize and get $$ alpha X^T X $$.
Maybe I am mistaken...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree with you, Max; we can control alpha by controlling W.
And for me, having W_q and W_v and stacking them on the OCP side makes more sense than having W_{state} (since it is consistent with TrajectoryPoint definition)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't warmstart know about the previous solution? I thought we assumed that OCP is stateless and warmstart is responsible for blending old solutions with incomming trajectory?
Maybe function:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So isn't that enough for all warm starts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to apologise for the confusion. It was supposed to be similarly to what @petrikvladimir suggested, but I messed up the name of the function. I wanted it to be:
Or something like this.
In reference to #97 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having access to the previous solution makes sense to me. Maybe we can optionally pass xs and us to generate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a commit related to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure about the
list[np.ndarray]
vslist[TrajectoryPoint]
This file was deleted.