Skip to content

Conversation

krshrimali
Copy link
Contributor

@krshrimali krshrimali commented Jul 13, 2021

This PR:

  1. Removes test_out skip: it's not needed anymore after it was fixed in addmv: port to structured kernels, improve error checks #55746. This should also close addmv_() allows resizing the tensor it operates on and produces wrong results #55589.
  2. Removes test_variant_consistency_eager skip, it was added by mistake in [testing] add broadcasts_input and verifies the behaviour for inplace_variant. #55771.
  3. Refines sample_inputs_addmv function, the updated function should now be cleaner and easy to read.

cc: @mruberry

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 13, 2021

💊 CI failures summary and remediations

As of commit dd0c1d4 (more details on the Dr. CI page and at hud.pytorch.org/pr/61579):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


Preview docs built from this PR

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.

Click here to manually regenerate this comment.

@krshrimali krshrimali changed the title Remove test_out skip for addmv; fixed before Remove test_out, test_variant_consistency_eager skips for addmv; fixed before Jul 13, 2021
@krshrimali krshrimali marked this pull request as draft July 13, 2021 13:57
@krshrimali krshrimali marked this pull request as ready for review July 13, 2021 15:34
@H-Huang H-Huang requested a review from mruberry July 13, 2021 22:04
@H-Huang H-Huang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 13, 2021
@krshrimali krshrimali added the module: testing Issues related to the torch.testing module (not tests) label Jul 14, 2021
broadcasts_input=broadcasts_input))
return tuple(sample_inputs)

def generator():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice refactor

@@ -4921,13 +4915,6 @@ def gradcheck_wrapper_triangular_input(op, input, *args, upper=False, **kwargs):
dtypesIfROCM=floating_types_and(torch.half),
supports_inplace_autograd=False,
supports_forward_ad=True,
skips=(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Always awesome to see skips removed

Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Great and necessary cleanup. Thanks @krshrimali!

@facebook-github-bot
Copy link
Contributor

@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@mruberry merged this pull request in f008e8d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged module: testing Issues related to the torch.testing module (not tests) open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

addmv_() allows resizing the tensor it operates on and produces wrong results
5 participants