Skip to content

Conversation

@albanD
Copy link
Collaborator

@albanD albanD commented May 24, 2017

Remaining to discuss:

  • Better testing strategy: right now the tests test everything that should work and should fail. Maybe we do not want to keep all these tests as they are quite slow?
    • Option 1: keep all of these but in a separate place so that we do not have to run all these tests every time but only if/when we modify this code.
    • Option 2: keep only a subset of these tests for specific stride/padding/dilation values.
    • Option 3: Something else?
  • Anything that would use dilated transpose convolution raises an explicit error. Do we want to keep this? Is the cudnn implementation of dilated transpose convolution correct and we want to possibly use it?
  • The case specified in the doc where part of the input is ignored due to striding is not always supported and raises an explicit error when it's not.
  • The Expand C Function is not différentiable.
  • For GPU support, few contiguous calls are missing.
  • transposed convolution are not supported at all and raises an explicit error.

@apaszke is any of these points blocking for merging this or can these be added afterward?

@ngimel
Copy link
Collaborator

ngimel commented May 24, 2017

Anything that would use dilated transpose convolution raises an explicit error. Do we want to keep this? >Is the cudnn implementation of dilated transpose convolution correct and we want to possibly use it?

Cudnn implementation looks ok at first sight, but I did not grad-check it. But we can't have some operation implemented with cudnn backend only?

@albanD
Copy link
Collaborator Author

albanD commented May 24, 2017

@ngimel well with the current implementation, trying to use dilated transposed convolution raises an error on CPU or on GPU when cudnn is disabled. But does something when cudnn is enabled.
In this particular case, the problem is that is raises an error that is hard to link to the problem and that is why I raise a meaningful error message (even in the case where using cudnn would have worked, but I'm not sure if it's a bug or a feature).

acc_inplace(var_grad, new_grad);
return variable_list();
}
if (new_grad) {

This comment was marked as off-topic.

static std::unique_ptr<Tensor> cat(const tensor_list& tensors, int dim) {
int num_inputs = tensors.size();
if (num_inputs == 0) {
return std::unique_ptr<Tensor>();

This comment was marked as off-topic.

// Add saved variables used out of the pure autograd to inputs
variable_list all_grad_outputs(grad_outputs);
all_grad_outputs.push_back(input_.unpack());
all_grad_outputs.push_back(weight_.unpack());

This comment was marked as off-topic.

return wrap_outputs(all_grad_outputs, std::move(outputs), [&](FunctionFlags f) {
return std::make_shared<ConvBackwardBackward>(
f, *this,
input_.unpack()->save(this), weight_.unpack()->save(this),

This comment was marked as off-topic.

static PyTypeObject ViewClass;
addClass<View, NoCtor>(module, ViewClass, "CView");
static PyTypeObject ExpandClass;
addClass<Expand, NoCtor>(module, ExpandClass, "CExpand");

This comment was marked as off-topic.

for padding in [0, 1, 2]:
for chan_in in [1, 3]:
for chan_out in [1, 3]:
for dilation in dilations:

This comment was marked as off-topic.


def test_conv_double_backward(self):
batch_size = 2
for kern, inp_size, dilations in [(3, 5, [1, 2]), (3, 7, [1, 2]), (4, 9, [2]), (4, 10, [1])]:

This comment was marked as off-topic.

return True


def gradgradcheck(func, inputs, grad_outputs, eps=1e-6, atol=1e-5, rtol=1e-3):

This comment was marked as off-topic.

This comment was marked as off-topic.

# "\nchan_out: " + str(chan_out) +
# "\nbatch_size: " + str(batch_size) +
# "\ninp_size: " + str(inp_size) +
# "\ndilation: " + str(dilation))

This comment was marked as off-topic.

else:
def func(x, weight, bias):
return F.conv2d(x, weight, bias, stride, padding, dilation)
inputs = (x, weight, bias,)

This comment was marked as off-topic.

@apaszke apaszke added this to the v0.2 milestone Jun 3, 2017
@aosokin
Copy link
Contributor

aosokin commented Jun 7, 2017

Hi @albanD @apaszke , what's the current status of this PR?

@apaszke
Copy link
Contributor

apaszke commented Jun 7, 2017

We'll merge it soon. I need to take another pass and review it, and should be good to go. It'll be in v0.2

if no_weight:
# Special case because transpose dilated convolution is not implemented
def func(x, bias):
return F.conv2d(x, weight, bias, stride, padding, dilation)

This comment was marked as off-topic.

This comment was marked as off-topic.

@mjdietzx
Copy link
Contributor

mjdietzx commented Jun 9, 2017

Hey, I tried out your branch, and I get this error:

RuntimeError: ConvNdBackwardBackward: expected 3 arguments (got 1) when calling loss.backward(). Where I am back propagating the gradient of ntwk w.r.t. input as is implemented here: https://github.com/caogang/wgan-gp/blob/master/gan_toy.py#L204.

Thanks for implementing this by the way! 👍

@albanD
Copy link
Collaborator Author

albanD commented Jun 9, 2017

@mjdietzx the branch was under development, this should be fixed now.

@mjdietzx
Copy link
Contributor

mjdietzx commented Jun 9, 2017

@albanD I re-compiled from latest commit c2c9e3d and I still get the same error (both when cudnn is enabled and disabled).

Copy link
Contributor

@apaszke apaszke 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, I only have a few minor comments. I didn't review the formulas, but they should be ok if gradgradcheck passes

This comment was marked as off-topic.

return True


def gradgradcheck(func, inputs, grad_outputs, eps=1e-6, atol=1e-5, rtol=1e-3):

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@soumith
Copy link
Contributor

soumith commented Jun 10, 2017

i also suggest rebase on top of master. alykhan added a lot of error checking code

@albanD albanD force-pushed the clean_conv_double_back branch from c2c9e3d to 828dd0e Compare June 13, 2017 09:48

This comment was marked as off-topic.

This comment was marked as off-topic.

tox.ini Outdated

This comment was marked as off-topic.

This comment was marked as off-topic.

test/test_nn.py Outdated
inputs = (x, bias,)
else:
def func(x, weight, bias):
# We disable cudnn during forward to avoid finite difference imprecision issues

This comment was marked as off-topic.

This comment was marked as off-topic.

@mjdietzx
Copy link
Contributor

@albanD, my model is resnet18 https://github.com/pytorch/vision/blob/master/torchvision/models/resnet.py. There is average pooling, max pooling and batch norm layers in addition to linear and conv. Should this still work? If it should work I will write up a simple script reproducing the error that i get.

File "/home/ubuntu/anaconda3/lib/python3.5/site-packages/torch/autograd/variable.py", line 151, in backward
torch.autograd.backward(self, gradient, retain_graph, create_graph, retain_variables)
  File "/home/ubuntu/anaconda3/lib/python3.5/site-packages/torch/autograd/__init__.py", line 98, in backward
    variables, grad_variables, retain_graph)
RuntimeError: ConvNdBackwardBackward: expected 3 arguments (got 1)```

@fmassa
Copy link
Member

fmassa commented Jun 14, 2017

There is not yet double backprop for average pooling, max pooling and batch norm, that might explain the errors you are seeing.

@apaszke
Copy link
Contributor

apaszke commented Jun 14, 2017

Even if that's the case, they should raise a different error in their backward

@soumith
Copy link
Contributor

soumith commented Jun 17, 2017

This is now merged into master via #1832
Thanks alban, for this fantastic PR!

@soumith soumith closed this Jun 17, 2017
@albanD albanD deleted the clean_conv_double_back branch June 20, 2017 12:56
@katerakelly
Copy link

Are there any plans to add support for higher order derivatives of transposed convolution?

@soumith
Copy link
Contributor

soumith commented Aug 16, 2017

yes, we definitely have plans. I'll do that over the next month or so.

houseroad added a commit to houseroad/pytorch that referenced this pull request Nov 29, 2018
…002d19

Summary:
Previous import was 882c5283c54345d131e8fe5c859e4844dcf7ca8e

Included changes:
- **[f461f7a](onnx/onnx@f461f7a)**: Show the op's type and name when the shape inference is failed. (pytorch#1623) <Jerry>
- **[ab8aaf9](onnx/onnx@ab8aaf9)**: Add scan test case (pytorch#1586) <G. Ramalingam>
- **[c95357e](onnx/onnx@c95357e)**: link the tutorial (pytorch#1650) <Lu Fang>
- **[d7e2420](onnx/onnx@d7e2420)**: Upgrade label encoder to support more input types (pytorch#1596) <Wei-Sheng Chin>
- **[6425108](onnx/onnx@6425108)**: Add Doc about Adding New Operator into ONNX (pytorch#1647) <Lu Fang>
- **[295889c](onnx/onnx@295889c)**: use an empty initializer to create map (pytorch#1643) <Lu Fang>
- **[e38f3ec](onnx/onnx@e38f3ec)**: Remove redundant const (pytorch#1639) <daquexian>
- **[ea694bf](onnx/onnx@ea694bf)**: implement fuse reduce->unsqueeze + fix assumption in nop_dropout pass (pytorch#1565) <Armen>
- **[6db386e](onnx/onnx@6db386e)**: make output shape clear enough for Softmax family (pytorch#1634) <Lu Fang>
- **[2b67c6e](onnx/onnx@2b67c6e)**: fix batchnorm doc (pytorch#1633) <Lu Fang>
- **[c901784](onnx/onnx@c901784)**: remove inappropriate consts (pytorch#1632) <Lu Fang>
- **[de82119](onnx/onnx@de82119)**: Shape inference fix for broadcast, concat and scan (pytorch#1594) <KeDengMS>
- **[d7ffe3b](onnx/onnx@d7ffe3b)**: Update Optimizer Docs (pytorch#1607) <Armen>
- **[d09d139](onnx/onnx@d09d139)**: mark PROTOBUF_INCLUDE_DIRS as BUILD_INTERFACE (pytorch#1466) <Yuta Okamoto>
- **[eb4b7c2](onnx/onnx@eb4b7c2)**: allow variadic parameters of different types (pytorch#1615) <G. Ramalingam>
- **[4166246](onnx/onnx@4166246)**: Fix onnxifi test (pytorch#1617) <Yinghai Lu>
- **[6706a4d](onnx/onnx@6706a4d)**: Fix a bug in vector address access (pytorch#1598) <Raymond Yang>
- **[ae39866](onnx/onnx@ae39866)**: Separate types of inputs 1 and 2 in OneHot op. (pytorch#1610) <Spandan Tiwari>
- **[45ba661](onnx/onnx@45ba661)**: Handle new types in the switch. (pytorch#1608) <Dmitri Smirnov>
- **[14853b6](onnx/onnx@14853b6)**: Bump docker image version to 230 used in CircleCI (pytorch#1606) <bddppq>
- **[e0993b8](onnx/onnx@e0993b8)**: [onnxifi] Make sure that backend handles run async. (pytorch#1599) <Roman Dzhabarov>
- **[e6965cc](onnx/onnx@e6965cc)**: Introduce SparseTensor ML proto (pytorch#1554) <Dmitri Smirnov>
- **[75b782f](onnx/onnx@75b782f)**: In driver test check the return status of onnxGetBackendIDs (pytorch#1597) <bddppq>
- **[c05b364](onnx/onnx@c05b364)**: Make CI log less verbose (pytorch#1595) <bddppq>
- **[fa568e4](onnx/onnx@fa568e4)**: Loop type shape inferencing (pytorch#1591) <Scott McKay>
- **[937e64c](onnx/onnx@937e64c)**: add uint8 (pytorch#1590) <Lu Fang>
- **[f86e951](onnx/onnx@f86e951)**: Add domain as an optional parameter for make_node function (pytorch#1588) <Young Kim>
- **[ff45588](onnx/onnx@ff45588)**: Remove unreachable code in shape_inference.h (pytorch#1585) <Changming Sun>
- **[f7dcad0](onnx/onnx@f7dcad0)**: Add several hyperbolic function ops. (pytorch#1499) <Sergii Dymchenko>
- **[a60ac7d](onnx/onnx@a60ac7d)**: Add OneHot op to ONNX. (pytorch#1567) <Spandan Tiwari>
- **[f6c3a7e](onnx/onnx@f6c3a7e)**: [compiler flag] Issue a warning if class has virtual method but missing virtual dtor. (pytorch#1583) <Roman Dzhabarov>
- **[88d1784](onnx/onnx@88d1784)**: Fix MaxUnpool shape inference when output_shape is provided as input (pytorch#1578) <Spandan Tiwari>
- **[20041b7](onnx/onnx@20041b7)**: Add type shape inferencing for the If operator (pytorch#1571) <Scott McKay>
- **[d6c4c75](onnx/onnx@d6c4c75)**: Add a virtual destructor to GraphInferencer (pytorch#1574) <Changming Sun>
- **[a339598](onnx/onnx@a339598)**: fix ConvTranspose spec (pytorch#1566) <Wenhao Hu>

Differential Revision: D13263831

fbshipit-source-id: 0c158dd12c45d704b6f37f63f3d74ed34ef2f534
facebook-github-bot pushed a commit that referenced this pull request Nov 30, 2018
…002d19 (#14568)

Summary:
Pull Request resolved: #14568

Previous import was 882c5283c54345d131e8fe5c859e4844dcf7ca8e

Included changes:
- **[f461f7a](onnx/onnx@f461f7a)**: Show the op's type and name when the shape inference is failed. (#1623) <Jerry>
- **[ab8aaf9](onnx/onnx@ab8aaf9)**: Add scan test case (#1586) <G. Ramalingam>
- **[c95357e](onnx/onnx@c95357e)**: link the tutorial (#1650) <Lu Fang>
- **[d7e2420](onnx/onnx@d7e2420)**: Upgrade label encoder to support more input types (#1596) <Wei-Sheng Chin>
- **[6425108](onnx/onnx@6425108)**: Add Doc about Adding New Operator into ONNX (#1647) <Lu Fang>
- **[295889c](onnx/onnx@295889c)**: use an empty initializer to create map (#1643) <Lu Fang>
- **[e38f3ec](onnx/onnx@e38f3ec)**: Remove redundant const (#1639) <daquexian>
- **[ea694bf](onnx/onnx@ea694bf)**: implement fuse reduce->unsqueeze + fix assumption in nop_dropout pass (#1565) <Armen>
- **[6db386e](onnx/onnx@6db386e)**: make output shape clear enough for Softmax family (#1634) <Lu Fang>
- **[2b67c6e](onnx/onnx@2b67c6e)**: fix batchnorm doc (#1633) <Lu Fang>
- **[c901784](onnx/onnx@c901784)**: remove inappropriate consts (#1632) <Lu Fang>
- **[de82119](onnx/onnx@de82119)**: Shape inference fix for broadcast, concat and scan (#1594) <KeDengMS>
- **[d7ffe3b](onnx/onnx@d7ffe3b)**: Update Optimizer Docs (#1607) <Armen>
- **[d09d139](onnx/onnx@d09d139)**: mark PROTOBUF_INCLUDE_DIRS as BUILD_INTERFACE (#1466) <Yuta Okamoto>
- **[eb4b7c2](onnx/onnx@eb4b7c2)**: allow variadic parameters of different types (#1615) <G. Ramalingam>
- **[4166246](onnx/onnx@4166246)**: Fix onnxifi test (#1617) <Yinghai Lu>
- **[6706a4d](onnx/onnx@6706a4d)**: Fix a bug in vector address access (#1598) <Raymond Yang>
- **[ae39866](onnx/onnx@ae39866)**: Separate types of inputs 1 and 2 in OneHot op. (#1610) <Spandan Tiwari>
- **[45ba661](onnx/onnx@45ba661)**: Handle new types in the switch. (#1608) <Dmitri Smirnov>
- **[14853b6](onnx/onnx@14853b6)**: Bump docker image version to 230 used in CircleCI (#1606) <bddppq>
- **[e0993b8](onnx/onnx@e0993b8)**: [onnxifi] Make sure that backend handles run async. (#1599) <Roman Dzhabarov>
- **[e6965cc](onnx/onnx@e6965cc)**: Introduce SparseTensor ML proto (#1554) <Dmitri Smirnov>
- **[75b782f](onnx/onnx@75b782f)**: In driver test check the return status of onnxGetBackendIDs (#1597) <bddppq>
- **[c05b364](onnx/onnx@c05b364)**: Make CI log less verbose (#1595) <bddppq>
- **[fa568e4](onnx/onnx@fa568e4)**: Loop type shape inferencing (#1591) <Scott McKay>
- **[937e64c](onnx/onnx@937e64c)**: add uint8 (#1590) <Lu Fang>
- **[f86e951](onnx/onnx@f86e951)**: Add domain as an optional parameter for make_node function (#1588) <Young Kim>
- **[ff45588](onnx/onnx@ff45588)**: Remove unreachable code in shape_inference.h (#1585) <Changming Sun>
- **[f7dcad0](onnx/onnx@f7dcad0)**: Add several hyperbolic function ops. (#1499) <Sergii Dymchenko>
- **[a60ac7d](onnx/onnx@a60ac7d)**: Add OneHot op to ONNX. (#1567) <Spandan Tiwari>
- **[f6c3a7e](onnx/onnx@f6c3a7e)**: [compiler flag] Issue a warning if class has virtual method but missing virtual dtor. (#1583) <Roman Dzhabarov>
- **[88d1784](onnx/onnx@88d1784)**: Fix MaxUnpool shape inference when output_shape is provided as input (#1578) <Spandan Tiwari>
- **[20041b7](onnx/onnx@20041b7)**: Add type shape inferencing for the If operator (#1571) <Scott McKay>
- **[d6c4c75](onnx/onnx@d6c4c75)**: Add a virtual destructor to GraphInferencer (#1574) <Changming Sun>
- **[a339598](onnx/onnx@a339598)**: fix ConvTranspose spec (#1566) <Wenhao Hu>

Reviewed By: zrphercule

Differential Revision: D13263831

fbshipit-source-id: a2ff22c6454e2430429e5a7d18d21661a7ffb0cb
jjsjann123 pushed a commit to jjsjann123/pytorch that referenced this pull request May 24, 2022
malfet pushed a commit that referenced this pull request Jun 8, 2022
Syncing nvfuser devel branch to upstream master. https://github.com/csarofeen/pytorch/

A few bigger updates:
1. Initial support of cp.async and cp.async.wait: csarofeen#1619
2. Emulate ampere's mma 16816 with Turing's mma 1688, for a unified interface: csarofeen#1643
3. Extending the infrastructure to support mma operators on turing and ampere arch: csarofeen#1440

Commits that's actually in this PR from the csarofeen branch
```
* dd23252 (csarofeen/devel) Fusion Segmenter: Unify single kernel and multi-kernel runtime path (#1710)
* b3d1c3f Fix missing cooperative launch (#1726)
* dc670a2 Async gmem copy support on sm80+ (#1619)
* 5e6a8da Add turing mma support and test (#1643)
* d6d6b7d Fix rFactor when there are indirect root domain(s), and refactor (#1723)
* 7093e39 Mma op integration on ampere (#1440)
* fade8da patch python test for bfloat16 (#1724)
* 8fbd0b1 Fine-grained kernel profiling (#1720)
* 77c1b4f Adding dry run mode to skip arch dependent checks (#1702)
* 151d95b More precise concretization analysis (#1719)
* f4d3630 Enable complex python tests (#1667)
* 4ceeee5 Minor bugfix in transform_rfactor.cpp (#1715)
* 3675c70 Separate root domain and rfactor domain in TransformPrinter (#1716)
* f68b830 Fix scheduling with polymorphic broadcast (#1714)
* 4ab5ef7 updating_ci_machine (#1718)
* 56585c5 Merge pull request #1711 from csarofeen/upstream_master_bump_0517
* 174d453 Allow using nvFuser on CUDA extension (#1701)
* 18bee67 Validate LOOP concrete IDs have complete IterDomains (#1676)
```
Pull Request resolved: #78244
Approved by: https://github.com/csarofeen, https://github.com/malfet
facebook-github-bot pushed a commit that referenced this pull request Jun 8, 2022
Summary:
Syncing nvfuser devel branch to upstream master. https://github.com/csarofeen/pytorch/

A few bigger updates:
1. Initial support of cp.async and cp.async.wait: csarofeen#1619
2. Emulate ampere's mma 16816 with Turing's mma 1688, for a unified interface: csarofeen#1643
3. Extending the infrastructure to support mma operators on turing and ampere arch: csarofeen#1440

Commits that's actually in this PR from the csarofeen branch
```
* dd23252 (csarofeen/devel) Fusion Segmenter: Unify single kernel and multi-kernel runtime path (#1710)
* b3d1c3f Fix missing cooperative launch (#1726)
* dc670a2 Async gmem copy support on sm80+ (#1619)
* 5e6a8da Add turing mma support and test (#1643)
* d6d6b7d Fix rFactor when there are indirect root domain(s), and refactor (#1723)
* 7093e39 Mma op integration on ampere (#1440)
* fade8da patch python test for bfloat16 (#1724)
* 8fbd0b1 Fine-grained kernel profiling (#1720)
* 77c1b4f Adding dry run mode to skip arch dependent checks (#1702)
* 151d95b More precise concretization analysis (#1719)
* f4d3630 Enable complex python tests (#1667)
* 4ceeee5 Minor bugfix in transform_rfactor.cpp (#1715)
* 3675c70 Separate root domain and rfactor domain in TransformPrinter (#1716)
* f68b830 Fix scheduling with polymorphic broadcast (#1714)
* 4ab5ef7 updating_ci_machine (#1718)
* 56585c5 Merge pull request #1711 from csarofeen/upstream_master_bump_0517
* 174d453 Allow using nvFuser on CUDA extension (#1701)
* 18bee67 Validate LOOP concrete IDs have complete IterDomains (#1676)
```

Pull Request resolved: #78244

Reviewed By: ejguan

Differential Revision: D36678948

Pulled By: davidberard98

fbshipit-source-id: 0ccde965acbd31da67d99c6adb2eaaa888948105
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.

9 participants