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.

Sign up for GitHub

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

Jump to bottom

Add Differentiable CEM solver #329

Merged
merged 35 commits into from
Mar 23, 2023
Merged

Add Differentiable CEM solver #329

merged 35 commits into from
Mar 23, 2023

Conversation

dishank-b
Copy link
Contributor

@dishank-b dishank-b commented Oct 17, 2022 •
edited
Loading

Motivation and Context

This adds a differential CEM solver to the optimizers class of Theseus. Can be used to solve control problems.
Based on https://github.com/facebookresearch/dcem

How Has This Been Tested

Passed the unit tests given.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

ToDos

  • Check with theseus_layer_test provided
  • Run with different backwards modes
  • Validate the gradients through the optimizer

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 17, 2022
@mhmukadam mhmukadam added the enhancement New feature or request label Oct 18, 2022
@dishank-b dishank-b marked this pull request as ready for review November 11, 2022 17:04
bamos
bamos suggested changes Dec 10, 2022
View reviewed changes
Copy link
Contributor

@bamos bamos left a comment

Choose a reason for hiding this comment

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

It looks like a nice initial version! I left a few minor comments inline to the core parts of the code and will review the notebooks in some more detail soon. And how are the tests looking?

tests/optimizer/nonlinear/test_dcem.py Outdated Show resolved Hide resolved
theseus/__init__.py Outdated Show resolved Hide resolved
theseus/optimizer/nonlinear/dcem.py Outdated Show resolved Hide resolved
theseus/optimizer/nonlinear/dcem.py Outdated Show resolved Hide resolved
theseus/optimizer/nonlinear/dcem.py Outdated Show resolved Hide resolved
@dishank-b
Copy link
Contributor Author

dishank-b commented Mar 8, 2023 •
edited
Loading

@luisenp @mhmukadam . Can you also have a look at this? I think it's ready to be merged from my side, a few other things can be done but that can be done in later PRs.

dishank-b added 18 commits March 8, 2023 16:00
@dishank-b
first implementation of dcem solver
31c277d
@dishank-b
dcem optimizer working
245d80c
@dishank-b
minor changes in dcem, added tests
11962c6
@dishank-b
online calculatino of n_batch
d069ee6
@dishank-b
initializing LML layer
24d2fa9
@dishank-b
dcem working backwards tutorial 2
5632588
@dishank-b
better vectorization in solver
655ba19
@dishank-b
vectoriztion in solve method for itr loop in optimizer class
63a04cf
@dishank-b
forward pass working perfectly with current set of hyperparams with b…
fd93941 
…atch 1
@dishank-b
dcem backward unit test passed for one setting
976fb08
@dishank-b
DCEM backward unit test working, not tested with leo, insanely slow w…
7547504 
…ith learnable error
@dishank-b
refactoring, removed DcemSolver in favour of solve method in DCEM opt…
3a22e09 
…imizer
@dishank-b
correcting circle ci errors
89c8b39
@dishank-b
corrected lml url for requirements.txt
9bf03c4
@dishank-b
corrected reuirements.txt for lml
a1064a2
@dishank-b
removing -e from requirements
c21dc69
@dishank-b
changing setup.py to install lml
c809e94
@dishank-b
changing setup.py to add lml
2bbf2db
@dishank-b dishank-b removed the request for review from suddhu March 10, 2023 19:18
luisenp
luisenp requested changes Mar 14, 2023
View reviewed changes
Copy link
Contributor

@luisenp luisenp left a comment

Choose a reason for hiding this comment

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

This looks great! Left a bunch of minor comments that should be easy to address. Thanks!

requirements/dev.txt Outdated Show resolved Hide resolved
requirements/main.txt Outdated Show resolved Hide resolved
tests/optimizer/nonlinear/test_dcem.py Outdated Show resolved Hide resolved
theseus/optimizer/nonlinear/dcem.py Outdated Show resolved Hide resolved
requirements/dev.txt Outdated Show resolved Hide resolved
theseus/optimizer/nonlinear/dcem.py Outdated Show resolved Hide resolved
theseus/optimizer/nonlinear/dcem.py Outdated Show resolved Hide resolved
theseus/optimizer/nonlinear/dcem.py Outdated Show resolved Hide resolved
tutorials/06_DCEM.ipynb Outdated Show resolved Hide resolved
tests/optimizer/nonlinear/test_dcem.py Outdated Show resolved Hide resolved
luisenp
luisenp reviewed Mar 20, 2023
View reviewed changes
Copy link
Contributor

@luisenp luisenp left a comment

Choose a reason for hiding this comment

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

Thanks for addressing these. Left a couple of follow up questions.

setup.py Outdated Show resolved Hide resolved
tests/test_theseus_layer.py Outdated Show resolved Hide resolved
theseus/optimizer/nonlinear/dcem.py Outdated Show resolved Hide resolved
tests/optimizer/nonlinear/test_dcem.py Outdated Show resolved Hide resolved
theseus/optimizer/nonlinear/dcem.py Show resolved Hide resolved
theseus/optimizer/nonlinear/dcem.py Outdated Show resolved Hide resolved
theseus/optimizer/nonlinear/dcem.py Outdated Show resolved Hide resolved
dishank-b
dishank-b commented Mar 21, 2023
View reviewed changes
Copy link
Contributor Author

@dishank-b dishank-b left a comment

Choose a reason for hiding this comment

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

Made the changes, have a look.

luisenp
luisenp approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@luisenp luisenp left a comment

Choose a reason for hiding this comment

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

LGTM! Left a couple of comments of things we can move to a separate PR. I think we could also rename some of the args/kwargs, but I'm OK with merging as is. I can take a look at this after we have a unit test in place. Thanks for your hard work on this!

theseus/optimizer/nonlinear/dcem.py Show resolved Hide resolved
tutorials/06_DCEM.ipynb Outdated Show resolved Hide resolved
tests/optimizer/nonlinear/test_dcem.py Outdated Show resolved Hide resolved
theseus/optimizer/nonlinear/dcem.py Outdated Show resolved Hide resolved
@luisenp luisenp mentioned this pull request Mar 22, 2023
Open
@luisenp luisenp merged commit 71abf01 into main Mar 23, 2023
@luisenp luisenp deleted the dishank.dcem branch March 23, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bamos bamos bamos requested changes

@luisenp luisenp luisenp approved these changes

@rtqichen rtqichen Awaiting requested review from rtqichen

@mhmukadam mhmukadam Awaiting requested review from mhmukadam

Assignees

@dishank-b dishank-b

Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dishank-b @bamos @luisenp @mhmukadam @facebook-github-bot