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

Extragradient and tests #21

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Extragradient and tests #21

wants to merge 22 commits into from

Conversation

manuel-delverme
Copy link
Collaborator

(This pull request depends on the HS suite)
adds:

  1. Extragradient optimizer and tests (18/20 passing)
  2. gradient descent/ascent, untested
  3. updates in the pip package
  4. a bug fix (?) https://github.com/gehring/fax/compare/master...manuel-delverme:extragradient_test?expand=1#diff-d6645bfd4bdc4d6e51625269e2018617R106

3 and 4 should be part of another PR, let me know if i should split this

pierrelux and others added 17 commits February 25, 2020 14:43
@pierrelux
Basic implementation of extra-gradient
3c4d321
@manuel-delverme
Merge remote-tracking branch 'manuel-delverme/master' into extragradient
157e2e3
@manuel-delverme
Added tests, forwarding the cost function for debugging purposes (not…
6a10a40 
… added to this commit)
@manuel-delverme
This commit includes only non functional changes, some of them might …
a2c0a8e 
…be personal preference/against the code style, let me know i will drop them
@manuel-delverme
added more details to setup.py
c8ae755
@manuel-delverme
added rprop extra gradient
fd79e66
@manuel-delverme
rprop EG solves ~half of the constrained tasks
e8accf6
@manuel-delverme
some tests fail, im not sure why
c2af669
@manuel-delverme
passing all the tests
927c37e
@manuel-delverme
Merge remote-tracking branch 'manuel-delverme/HockSchittkowski_tests'…
967f73e 
… into extragradient_test

# Conflicts:
#	.gitignore
#	fax/constrained/constrained_test.py
#	fax/test_util.py
@manuel-delverme
3/17 tests fail
bc12646
@manuel-delverme
non working extragradient.py cleanup
bf6d58c
@manuel-delverme
2/20 tests fail
0b3a821
@manuel-delverme
some cleanup
139b9f5
@manuel-delverme
removing basic tests, the HS test suite is broad enough;
fa21e13 
gitignore cleanup
@manuel-delverme
removed extragradient_test.py, the tests are in constrained_test.py
da1baa3
@manuel-delverme
Merge branch 'master' into extragradient_test
5350c21
@gehring
Copy link
Owner

gehring commented Jun 3, 2020

Can you open an issue with a brief explanation of what the bug is. It's not obvious to me from your link.

Could you elaborate on what is preventing us from passing 2/20 tests?

@gehring gehring self-requested a review June 3, 2020 23:20
@pierrelux pierrelux self-requested a review June 5, 2020 03:26
pierrelux
pierrelux requested changes Jun 5, 2020
View reviewed changes
Copy link
Collaborator

@pierrelux pierrelux 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 your work. General comment: this PR should really be split into smaller ones. Also, there seems to be a duplicate of code with #14. I also realized that #14 changes the CGA-related code to add type annotation and other auto-formatting changes: I would keep that separate as well and we should generally try to configure our IDEs so that we don't re-format everything on save.

fax/competitive/cga.py Outdated Show resolved Hide resolved
fax/competitive/sgd.py Outdated Show resolved Hide resolved
fax/constrained/constrained.py Outdated Show resolved Hide resolved
fax/constrained/constrained.py Outdated Show resolved Hide resolved
fax/constrained/constrained_test.py Outdated Show resolved Hide resolved
fax/loop.py Outdated Show resolved Hide resolved
fax/loop_test.py Show resolved Hide resolved
fax/test_util.py Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
fax/test_util.py Show resolved Hide resolved
@manuel-delverme
Copy link
Collaborator Author

2/20 tests
Here I'm using a set of hyperparameters for all the examples,
I think i would have to create per-test hyperparameters to reach 20/20, it did not seem important

@manuel-delverme manuel-delverme linked an issue Jun 8, 2020 that may be closed by this pull request
Extragradient solver #17
Open
@gehring
Copy link
Owner

gehring commented Jun 29, 2020

@pierrelux Have all your requested changes been addressed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NeilGirdhar NeilGirdhar NeilGirdhar left review comments

@gehring gehring Awaiting requested review from gehring

@pierrelux pierrelux Awaiting requested review from pierrelux

Assignees

@manuel-delverme manuel-delverme

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extragradient solver
4 participants
@manuel-delverme @gehring @pierrelux @NeilGirdhar