Skip to content

Conversation

@ssnl
Copy link
Collaborator

@ssnl ssnl commented Dec 22, 2017

Fixes #4217

This PR breaks btrifact into btrifact and btrifact_with_info. The cwrap declarations for for tensor and variable are different in that the tensor.btrifact in TensorMath.cwrap still allows info optional argument, and var.btrifact in Decalaration.cwrap always sets info as NULL. In variable.py, a python method btrifact is added to support old info optional argument behavior, but I added deprecation warning in favor of btrifact_with_info.

I also modified preprocess_declarations.py's is_nullable for optional tensor arguments with NULL default values. An example of input declarations is the old btrifactin Declaration.cwrap:

  name: btrifact
  cname: btrifact
  types:
    - floating_point
  backends:
    - CPU
    - CUDA
  variants:
    - method
    - function
  return: argument 0,1
  arguments:
    - arg: THTensor* result
      output: True
    - arg: THIntegerTensor* pivots
      output: True
    - arg: THIntegerTensor* info
      kwarg_only: True
      default: NULL
    - arg: bool pivot
      kwarg_only: True
      default: "true"
    - THTensor* self
]]

, which gets parsed into the following:

{..., 'options': [{'arguments': [..., 
{'kwarg_only': True, 'default': None, 'type': 'THInteger Tensor*', 'name': 'info'}, 
...] ...}

@pytorchbot
Copy link
Collaborator

@ssnl, thanks for your PR! We identified @zdevito to be a potential reviewer.

Copy link
Member

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

looks good. can you make sure there's a test that calls btrifact_with_info?

@ssnl
Copy link
Collaborator Author

ssnl commented Dec 22, 2017

Added tests with btrifact_with_info, doc for btriunpack and tensor.btrifact*.

@ezyang
Copy link
Contributor

ezyang commented Dec 22, 2017

Protip: if you want to pretty print the structs, use

import pprint
pprint.prrint(foo)

@apaszke
Copy link
Contributor

apaszke commented Dec 22, 2017

cc: @bamos @zkolter as this might be breaking for qpth or some other projects

@ssnl
Copy link
Collaborator Author

ssnl commented Dec 22, 2017 via email

@ssnl
Copy link
Collaborator Author

ssnl commented Dec 23, 2017

@pytorchbot retest this please

@apaszke
Copy link
Contributor

apaszke commented Dec 23, 2017

Not in the short term, but it will be removed at some point

@ssnl
Copy link
Collaborator Author

ssnl commented Dec 23, 2017

@pytorchbot retest this please

@soumith soumith merged commit 9a48f8d into pytorch:master Dec 23, 2017
@ssnl ssnl deleted the btrifact_var branch December 23, 2017 22:32
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.

btrifact fails with Variable arguments

6 participants