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

V0.1.0 of Lava-optimization #7

Merged
merged 87 commits into from
Nov 13, 2021
Merged

V0.1.0 of Lava-optimization #7

merged 87 commits into from
Nov 13, 2021

Conversation

ashishrao7
Copy link
Collaborator

@ashishrao7 ashishrao7 commented Nov 10, 2021

This pull request is v0.1.0 of the lava-optimization library. It references issues #2 and #3 . It contains the first version of the QP solver and a tutorial for using QP solvers to implement LASSO.
Reviewers: @GaboFGuerra @awintel and Sumedh

GaboFGuerra and others added 30 commits November 3, 2021 16:11
Signed-off-by: gguerra <gabriel.guerra@intel.com>
… Need to test them with the developing solver interface
QP process and models (without tests
)
… except constraintcheck and gradient dynamics
…d Initialization tests to test_process.py and behavioral tests to test_models.py
… level in models.py. Pending GradientDynamics behavioral test
@ashishrao7
Copy link
Collaborator Author

ashishrao7 commented Nov 12, 2021

Great! I already approved but now there is one last thing...

You argued before that the users of the library would be intimately familiar with the member names Q, p, etc. and that it would not be helpful to rename them to more descriptive names. The @property methods for those members have descriptive names now (get_hessian() returns self._Q). I suggest sticking either with the math-inspired or the descriptive names throughout to be consistent.

I changed this to be more consistent like you suggested. Matched it to the interface.

Apart from that everything looks good.

Also added typing now

Done in commit 7313974

mgkwill
mgkwill previously approved these changes Nov 12, 2021
Copy link
Collaborator

@mgkwill mgkwill left a comment

Choose a reason for hiding this comment

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

Looks good to me (LGTM).

setup.py is auto generated, but formatting it is fine.

For consistency sake:
src/lava/lib/optimization/init.py does not have copyright header.
src/lava/lib/optimization/solvers/init.py does not have copyright header.

I don't think we should have src/init.py, but we can test without and fix later

After our conversation we should follow lowercase var names in src/lava/lib

@ashishrao7
Copy link
Collaborator Author

Looks good to me (LGTM).

setup.py is auto generated, but formatting it is fine.

For consistency sake: src/lava/lib/optimization/init.py does not have copyright header. src/lava/lib/optimization/solvers/init.py does not have copyright header.

I don't think we should have src/init.py, but we can test without and fix later

After our conversation we should follow lowercase var names in src/lava/lib

Resolved all comment in commit b09944e

Copy link
Contributor

@phstratmann phstratmann left a comment

Choose a reason for hiding this comment

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

My comments were all handled well and resolved, thanks Ashish!

Please note: Due to time restrictions, I only covered ~50% of the code and not to full depth.

Copy link
Collaborator

@GaboFGuerra GaboFGuerra left a comment

Choose a reason for hiding this comment

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

Looks good!

Signed-off-by: Marcus G K Williams <marcus.williams@intel.com>
@mgkwill
Copy link
Collaborator

mgkwill commented Nov 13, 2021

Only failure in CI is because of long path on windows. False positive. Merging.

@mgkwill mgkwill merged commit c1c197f into lava-nc:main Nov 13, 2021
@GaboFGuerra GaboFGuerra deleted the v0.1.0 branch January 21, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants