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

added typing to linear_algebra.py #413

Merged
merged 4 commits into from
Oct 5, 2022
Merged

added typing to linear_algebra.py #413

merged 4 commits into from
Oct 5, 2022

Conversation

shivance
Copy link
Contributor

I added types to linear_algebra.py

I'm not sure whether we should add types to nested private functions, so I haven't for now.
Please review !

(#250 )

@google-cla
Copy link

google-cla bot commented Sep 11, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@shivance
removing unused optparse import
8f5560a 
this was imported mistakenly, is not used in the code
mkunesch
mkunesch reviewed Sep 18, 2022
View reviewed changes
Copy link
Member

@mkunesch mkunesch left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR!

optax/_src/linear_algebra.py Outdated Show resolved Hide resolved
optax/_src/linear_algebra.py Outdated Show resolved Hide resolved
optax/_src/linear_algebra.py Outdated Show resolved Hide resolved
mkunesch
mkunesch requested changes Sep 18, 2022
View reviewed changes
Copy link
Member

@mkunesch mkunesch left a comment

Choose a reason for hiding this comment

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

(sorry, forgot to tick the box to mark this as reviewed)

@shivance
Copy link
Contributor Author

@mkunesch Thanks for the feedback . Got the point over there wrt Optional !
Performed the requested change.

hbq1
hbq1 reviewed Oct 4, 2022
View reviewed changes
optax/_src/linear_algebra.py Outdated Show resolved Hide resolved
@shivance
Copy link
Contributor Author

shivance commented Oct 4, 2022 •
edited
Loading

@hbq1 @mkunesch I have removed the unused import ! and the PR is ready for next round of review once the CI passes.

@shivance shivance requested review from mkunesch and hbq1 and removed request for mkunesch and hbq1 October 4, 2022 16:23
hbq1
hbq1 approved these changes Oct 4, 2022
View reviewed changes
Copy link
Collaborator

@hbq1 hbq1 left a comment

Choose a reason for hiding this comment

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

Thanks!

@shivance
Copy link
Contributor Author

shivance commented Oct 4, 2022

@hbq1 the CI turned out to be clean .

@hbq1
Copy link
Collaborator

hbq1 commented Oct 4, 2022

Nice! Waiting for approval from @mkunesch

@copybara-service copybara-service bot merged commit d744b3d into google-deepmind:master Oct 5, 2022
@hbq1 hbq1 mentioned this pull request Oct 6, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkunesch mkunesch Awaiting requested review from mkunesch

@hbq1 hbq1 Awaiting requested review from hbq1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@shivance @hbq1 @mkunesch