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

Cleaned up sparse solvers code #386

Merged
merged 5 commits into from
Nov 29, 2022
Merged

Cleaned up sparse solvers code #386

merged 5 commits into from
Nov 29, 2022

Conversation

luisenp
Copy link
Contributor

@luisenp luisenp commented Nov 29, 2022

Did some clean up of the code in optimizer/autograd and optimizer/linear, including adding type hints. Most of the changes are variable names and type hints, except for adding new_empty in a couple of places, and adding new function for samping sparse matrices.

There is some duplicated code in backward passes that should be relatively easy to fix, but I'd prefer doing that in a separate smaller PR to avoid introducing bugs.

Closes #326

@luisenp luisenp requested a review from maurimo November 29, 2022 03:28
@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 Nov 29, 2022
Copy link
Contributor

@maurimo maurimo left a comment

Choose a reason for hiding this comment

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

Changes look great, if CI passes I'm ok with merging this PR!

@luisenp luisenp merged commit adb7c70 into main Nov 29, 2022
@luisenp luisenp deleted the lep.cleanup_sparse_code branch November 29, 2022 13:11
Copy link
Member

@mhmukadam mhmukadam left a comment

Choose a reason for hiding this comment

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

LGTM!

suddhu pushed a commit to suddhu/theseus that referenced this pull request Jan 21, 2023
* Created utility function to sample sparse matrix tensors.

* Renamed some incorrectly capitalized variables.

* Added missing type hints to sparse linear solvers code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add missing type hints in /theseus/optimizer
4 participants