Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: make root_vjp public #157

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Benjamin-eecs
Copy link
Member

Description

make root_vjp public to users

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • New feature (non-breaking change which adds core functionality)
  • Documentation (update in the documentation)

Checklist

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

  • I have read the CONTRIBUTION guide. (required)
  • My change requires a change to the documentation.
  • I have updated the tests accordingly. (required for a bug fix or a new feature)
  • I have updated the documentation accordingly.
  • I have reformatted the code using make format. (required)
  • I have checked the code using make lint. (required)
  • I have ensured make test pass. (required)

@Benjamin-eecs Benjamin-eecs requested a review from JieRen98 April 15, 2023 01:27
@Benjamin-eecs Benjamin-eecs self-assigned this Apr 15, 2023
@Benjamin-eecs Benjamin-eecs requested a review from XuehaiPan April 15, 2023 01:27
@codecov
Copy link

codecov bot commented Apr 15, 2023

Codecov Report

Patch coverage: 92.31% and project coverage change: -0.01 ⚠️

Comparison is base (a6a2ed1) 93.37% compared to head (2fd5166) 93.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #157      +/-   ##
==========================================
- Coverage   93.37%   93.36%   -0.01%     
==========================================
  Files          71       72       +1     
  Lines        2684     2696      +12     
==========================================
+ Hits         2506     2517      +11     
- Misses        178      179       +1     
Flag Coverage Δ
unittests 93.36% <92.31%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
torchopt/diff/implicit/decorator.py 95.73% <ø> (ø)
torchopt/diff/implicit/utils.py 90.91% <90.91%> (ø)
torchopt/diff/implicit/__init__.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@XuehaiPan XuehaiPan left a comment

Choose a reason for hiding this comment

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

Need more design for the vjp API.

  • root_vjp should return a function rather than the product result.
  • How do you ensure the solution argument is the root of solve_fn? Seems that the user can pass arbitrary tensors to your root_vjp API.
  • Need more design for argnums and argument binding. Need more investigation for functools.partial support.

@XuehaiPan XuehaiPan added enhancement New feature or request feature New feature labels Apr 25, 2023
@Benjamin-eecs Benjamin-eecs added this to the 0.8.0 milestone May 12, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants