-
Notifications
You must be signed in to change notification settings - Fork 175
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
Added LWA frame convention in Contact1D #1172
Conversation
63a3296
to
1a8f2ac
Compare
Hi @skleff1994, Thanks for creating again this PR! Could you check locally that all tests pass before pushing the code? |
Hi @cmastalli Thank you for reviewing this PR. Sorry about that, but I don't understand why the CI is not passing. All unittests pass locally on my machine (in Debug mode) as seen below
I compiled with the following flags on Ubuntu 20.04 Can you please let me know how I can debug the checks that do not pass? |
Dear @skleff1994 you can click on Details links related to the checks. 1/ You have formatting problems which are easily fixed by installing pre-commit on your machine. Please see: https://pre-commit.com/ This is allowing an uniform C++ coding style whatever is your favorite IDE. 2/ Checking simply on the Details you will see that for 22.04 on the LAAS CI:
It is related to test |
|
unittest/bindings/test_copy.py
Outdated
@@ -317,7 +317,13 @@ class ContactsTest(CopyModelTestCase): | |||
MODEL.append(crocoddyl.ContactModelMultiple(state, actuation.nu)) | |||
COLLECTOR.append(pdata) | |||
MODEL.append( | |||
crocoddyl.ContactModel1D(state, frame_id, 1.0, actuation.nu, np.zeros(2)) | |||
crocoddyl.ContactModelAbstract(state, pinocchio.LOCAL, 3, actuation.nu) |
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.
Remove this line. This should fix errors in the CI.
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.
Thanks for writing a clear PR, @skleff1994
Everything looks alright to me.
However, I highly suggest extending this contact model to an arbitrary constraint axis.
What do you think? See my comment below.
} | ||
switch (type_) { | ||
case pinocchio::ReferenceFrame::LOCAL: | ||
d->da0_dx.row(0) = d->da0_local_dx.row(2); |
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.
Should we always assume constraints in Z?
It would be useful if this could be configured via a rotation matrix moving the z-axis.
This will require
- including this information in the constructors and
- writing unit tests with random rotation matrix.
Could you handle 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.
Yes I agree . I can handle 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.
Please ping me when this is done. Thanks!
4165ad0
to
8cdecdb
Compare
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 summary of actions is:
- Revert the proposed change in the constructor API to a vector
- Include a new constructor where the rotation matrix is assumed to be the identity matrix.
- Clean up some added files.
- Adapt the unit-test and Python contact models as suggested in points 1 and 2.
Please ping me when you're done.
Thanks!
d3cfbaa
to
fc4c5e7
Compare
@cmastalli I addressed the changes you requested, however I'm not sure what happened - my 2 local commits turned into thousands of commits in this PR . Is it related to the rewriting of the git history you suggested in your comment #1172 (comment) ? I did : I will try a git rebase |
I understand. Please remove this commit and then make a commit that removes the file. This should solve the problem. |
fc4c5e7
to
ad3b0b5
Compare
@cmastalli I deleted the commit but it didn't solve the issue I will squash my commits and cherry-pick after a git reset --hard onto devel |
… unittests fixed Contact1D unittests in LWA / WORLD fixed contact1d in python test remove ContactModelAbstract from copy test + changed HAL link to paper ref in contact models 1d, 3d, 6d added rotation matrix for contact1D + updated bindings + unittests accordingly fixed 1d force derivatives fixed 1D force derivatives with rotation fixed bindings and unittests for contact1d removed content of __init__.py and fixed import in contact python test
ad3b0b5
to
38a6e59
Compare
for more information, see https://pre-commit.ci
Ok I believe the rebase + reset + cherry-pick fixed the issue @cmastalli |
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.
Thanks, @skleff1994 !
It looks like everything is alright.
Let's merge this if the CI passes.
Upstream changes: ## [2.0.2] - 2023-12-07 * Added nu, ng, and nh setters for Python bindings in loco-3d/crocoddyl#1192 * Added CHANGELOG.md in loco-3d/crocoddyl#1188 * Supported nu==0 in actuation models in loco-3d/crocoddyl#1188 * Included Python bindings for Crocoddyl exceptions by cmastalli in loco-3d/crocoddyl#1186 * Updated cmake submodule update by jcarpentier in loco-3d/crocoddyl#1186 * Fixed getters for contraints bounds by skleff1994 in loco-3d/crocoddyl#1180 * Extended solver abstract and callbacks for arbitrary solvers by cmastalli in loco-3d/crocoddyl#1179 * Fixed the check of pair_id in collision residual by ArthurH91 in loco-3d/crocoddyl#1178 * Exploited control-residual structure when computing Lu, Luu by cmastalli in loco-3d/crocoddyl#1176 * Added LWA fram convention and introduced different axis for 1d contacts by skleff1994 in loco-3d/crocoddyl#1172 * Python bindings for setting control bounds by cmastalli in loco-3d/crocoddyl#1171 * Fixed missed scalar in cost sum and activation data by cmastalli in loco-3d/crocoddyl#1165 * Added actuation unit tests by cmastalli in loco-3d/crocoddyl#1161 * Introduced method for obtaining the dimension of floating-bases by cmastalli in loco-3d/crocoddyl#1160 * Fixed set_reference in state residual by cmastalli in loco-3d/crocoddyl#1158 * Enabled CONDA CI jobs with CppADCodeGen by cmastalli in loco-3d/crocoddyl#1156 * Added other CI jobs by cmastalli in loco-3d/crocoddyl#1152 * Fixed compiltation issue when building with CppADCodeGen by cmastalli in loco-3d/crocoddyl#1151 * Fixed include order used in frames.cpp by ManifoldFR in loco-3d/crocoddyl#1150
Upstream changes: ## [2.0.2] - 2023-12-07 * Added nu, ng, and nh setters for Python bindings in loco-3d/crocoddyl#1192 * Added CHANGELOG.md in loco-3d/crocoddyl#1188 * Supported nu==0 in actuation models in loco-3d/crocoddyl#1188 * Included Python bindings for Crocoddyl exceptions by cmastalli in loco-3d/crocoddyl#1186 * Updated cmake submodule update by jcarpentier in loco-3d/crocoddyl#1186 * Fixed getters for contraints bounds by skleff1994 in loco-3d/crocoddyl#1180 * Extended solver abstract and callbacks for arbitrary solvers by cmastalli in loco-3d/crocoddyl#1179 * Fixed the check of pair_id in collision residual by ArthurH91 in loco-3d/crocoddyl#1178 * Exploited control-residual structure when computing Lu, Luu by cmastalli in loco-3d/crocoddyl#1176 * Added LWA fram convention and introduced different axis for 1d contacts by skleff1994 in loco-3d/crocoddyl#1172 * Python bindings for setting control bounds by cmastalli in loco-3d/crocoddyl#1171 * Fixed missed scalar in cost sum and activation data by cmastalli in loco-3d/crocoddyl#1165 * Added actuation unit tests by cmastalli in loco-3d/crocoddyl#1161 * Introduced method for obtaining the dimension of floating-bases by cmastalli in loco-3d/crocoddyl#1160 * Fixed set_reference in state residual by cmastalli in loco-3d/crocoddyl#1158 * Enabled CONDA CI jobs with CppADCodeGen by cmastalli in loco-3d/crocoddyl#1156 * Added other CI jobs by cmastalli in loco-3d/crocoddyl#1152 * Fixed compiltation issue when building with CppADCodeGen by cmastalli in loco-3d/crocoddyl#1151 * Fixed include order used in frames.cpp by ManifoldFR in loco-3d/crocoddyl#1150
This PR just adds the LWA / WORLD convention in the Contact1D. I also updated the bindings & unittests accordingly (the Contact1D model was only available in LOCAL).
Note: This completes the unfinished work of #1054