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

Beef up {jacobian, hessian} vectorize docs; eliminate a warning #51638

Closed
wants to merge 1 commit into from

Conversation

zou3519
Copy link
Contributor

@zou3519 zou3519 commented Feb 3, 2021

Stack from ghstack:

This PR makes the following doc changes:

  • Makes it clear to users that they should use vectorize "at their own
    risk"
  • Makes it clear that vectorize uses the "experimental prototype vmap"
    so that when users see error messages related to vmap they will know
    where it is coming from.

This PR also:

  • makes it so that {jacobian, hessian} call a version of vmap that
    doesn't warn the user that they are using an "experimental prototype".
    The regular torch.vmap API does warn the user about this. This is to
    improve a UX a little because the user already knows from discovering
    the flag and reading the docs what they are getting themselves into.

Test Plan:

  • Add test that {jacobian, hessian} with vectorize=True don't raise
    warnings

Differential Revision: D26225402

This PR makes the following doc changes:
- Makes it clear to users that they should use vectorize "at their own
risk"
- Makes it clear that vectorize uses the "experimental prototype vmap"
so that when users see error messages related to vmap they will know
where it is coming from.

This PR also:
- makes it so that {jacobian, hessian} call a version of vmap that
doesn't warn the user that they are using an "experimental prototype".
The regular torch.vmap API does warn the user about this. This is to
improve a UX a little because the user already knows from discovering
the flag and reading the docs what they are getting themselves into.

Test Plan:
- Add test that {jacobian, hessian} with vectorize=True don't raise
warnings

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Feb 3, 2021

💊 CI failures summary and remediations

As of commit 0cb72ed (more details on the Dr. CI page):


  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

zou3519 added a commit that referenced this pull request Feb 3, 2021
This PR makes the following doc changes:
- Makes it clear to users that they should use vectorize "at their own
risk"
- Makes it clear that vectorize uses the "experimental prototype vmap"
so that when users see error messages related to vmap they will know
where it is coming from.

This PR also:
- makes it so that {jacobian, hessian} call a version of vmap that
doesn't warn the user that they are using an "experimental prototype".
The regular torch.vmap API does warn the user about this. This is to
improve a UX a little because the user already knows from discovering
the flag and reading the docs what they are getting themselves into.

Test Plan:
- Add test that {jacobian, hessian} with vectorize=True don't raise
warnings

ghstack-source-id: 51cc04dbc16c2cf45f0ee403ccaa44bc08081f70
Pull Request resolved: #51638
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks for the update!
The new phrasing will help reduce confusion for sure!

@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #51638 (0cb72ed) into gh/zou3519/347/base (a990ff7) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@                   Coverage Diff                   @@
##           gh/zou3519/347/base   #51638      +/-   ##
=======================================================
- Coverage                80.87%   80.87%   -0.01%     
=======================================================
  Files                     1943     1943              
  Lines                   211692   211694       +2     
=======================================================
- Hits                    171208   171199       -9     
- Misses                   40484    40495      +11     

@facebook-github-bot
Copy link
Contributor

@zou3519 merged this pull request in 45e5562.

@facebook-github-bot facebook-github-bot deleted the gh/zou3519/347/head branch February 7, 2021 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants