'fuse_conv_bn_weights' and 'fuse_linear_bn_weights' in 'torch.nn.utils'

[yueyinqiu]

Hello! I've tried to add two methods fuse_conv_bn_weights and fuse_linear_bn_weights in torch.nn.utils.

Since it's my first pull request that modifies the code, could someone please give me some advice? In fact I'm not sure about the following things:

• Am I required to add a high coverage unit test? Meanwhile, where could I put some tests?
  • I've found some similar methods like clip_grad_value_ does not have a related unit test, and parameters_to_vector's tests are placed in TestTorchSharp and named UtilsPtoV, so I'm a bit confused with that.
  • (Actually I haven't check whether the two methods could work correctly, although I believe they do, since what I have done is just translating the python codes into csharp.)
• Should I always dispose all the temporary tensors, or just leave it there (and wait for GC or dispose together with the outer dispose scope if there is one)?
  • I've found many methods does not deal with this, especially in torchaudio.transform. Meanwhile most functions just invoke the native code and returns the result without any extra tensors. So I'm not sure whether torchaudio is an exception or it is recommend to leave these tensors aside.
• Should I check the arguments and throw exception in advance (like whether they are null, or whether a tensor' shape is expected), or just let it continue until there is an inner exception thrown?

[yueyinqiu]

Actually I’ve also tried to implement fuse_conv_bn_eval and fuse_linear_bn_eval, but I don't know how to do fused_conv = copy.deepcopy(conv) in C#. Is there an existing method that could make such a copy of a module?

[NiklasGustafsson]

Am I required to add a high coverage unit test?

That is a best practice, but one that we haven't always followed, and the existing tests aren't always "high coverage." I will be reluctant to merge without some tests. It's a little bit complicated to add a new file, so just find a test file with similar functionality and add tests there.

Should I always dispose all the temporary tensors

That depends on how many there are -- if there's just one or two, let a surrounding dispose scope take care of it. If there's more than just a couple, then place a dispose scope at the beginning and escape the function result when it's returned. In a few rare cases, such as when there's a loop and temporaries are created in the body of the loop, you should place a dispose scope inside the loop, too.

Should I check the arguments and throw exception in advance

Yes. This is another area where we haven't been consistent. Explicit argument checking is always better, since the error message can be more precise and actionable.

Also, please edit the RELEASENOTES.md file. If the top release heading is what is current on NuGet, then add a new release. There should be a 'Fixed Bugs' heading, and an 'API Changes' heading. Place a '<br/>' after each item in any list of additions/fixes (GH's MD renderer won't separate the lines without it).

[yueyinqiu]

Yes, sure, a unit test to show the methods could work in most ordinary cases is necessary. I will mark this as a draft until the things mentioned above have been completed.

[yueyinqiu]

• basic unit tests
• tensor disposal
• RELEASENOTES.md

[NiklasGustafsson]

From the CI/CD build:

/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(283,30): error CS1026: ) expected [/__w/1/s/test/TorchSharpTest.WithCudaBinaries/TorchSharpTest.WithCudaBinaries.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(283,38): error CS1002: ; expected [/__w/1/s/test/TorchSharpTest.WithCudaBinaries/TorchSharpTest.WithCudaBinaries.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(283,38): error CS1513: } expected [/__w/1/s/test/TorchSharpTest.WithCudaBinaries/TorchSharpTest.WithCudaBinaries.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(314,30): error CS1026: ) expected [/__w/1/s/test/TorchSharpTest.WithCudaBinaries/TorchSharpTest.WithCudaBinaries.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(314,46): error CS1002: ; expected [/__w/1/s/test/TorchSharpTest.WithCudaBinaries/TorchSharpTest.WithCudaBinaries.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(314,46): error CS1513: } expected [/__w/1/s/test/TorchSharpTest.WithCudaBinaries/TorchSharpTest.WithCudaBinaries.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(283,30): error CS1026: ) expected [/__w/1/s/test/TorchSharpTest/TorchSharpTest.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(283,38): error CS1002: ; expected [/__w/1/s/test/TorchSharpTest/TorchSharpTest.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(283,38): error CS1513: } expected [/__w/1/s/test/TorchSharpTest/TorchSharpTest.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(314,30): error CS1026: ) expected [/__w/1/s/test/TorchSharpTest/TorchSharpTest.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(314,46): error CS1002: ; expected [/__w/1/s/test/TorchSharpTest/TorchSharpTest.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(314,46): error CS1513: } expected [/__w/1/s/test/TorchSharpTest/TorchSharpTest.csproj]

[yueyinqiu]

From the CI/CD build:

/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(283,30): error CS1026: ) expected [/__w/1/s/test/TorchSharpTest.WithCudaBinaries/TorchSharpTest.WithCudaBinaries.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(283,38): error CS1002: ; expected [/__w/1/s/test/TorchSharpTest.WithCudaBinaries/TorchSharpTest.WithCudaBinaries.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(283,38): error CS1513: } expected [/__w/1/s/test/TorchSharpTest.WithCudaBinaries/TorchSharpTest.WithCudaBinaries.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(314,30): error CS1026: ) expected [/__w/1/s/test/TorchSharpTest.WithCudaBinaries/TorchSharpTest.WithCudaBinaries.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(314,46): error CS1002: ; expected [/__w/1/s/test/TorchSharpTest.WithCudaBinaries/TorchSharpTest.WithCudaBinaries.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(314,46): error CS1513: } expected [/__w/1/s/test/TorchSharpTest.WithCudaBinaries/TorchSharpTest.WithCudaBinaries.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(283,30): error CS1026: ) expected [/__w/1/s/test/TorchSharpTest/TorchSharpTest.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(283,38): error CS1002: ; expected [/__w/1/s/test/TorchSharpTest/TorchSharpTest.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(283,38): error CS1513: } expected [/__w/1/s/test/TorchSharpTest/TorchSharpTest.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(314,30): error CS1026: ) expected [/__w/1/s/test/TorchSharpTest/TorchSharpTest.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(314,46): error CS1002: ; expected [/__w/1/s/test/TorchSharpTest/TorchSharpTest.csproj]
/__w/1/s/test/TorchSharpTest/TestTorchSharp.cs(314,46): error CS1513: } expected [/__w/1/s/test/TorchSharpTest/TorchSharpTest.csproj]

I'm not sure what's happening. It definitely could work on my laptop. Maybe I should have some extra commits to test CI/CD's behavior.

[yueyinqiu]

oh.. that is the collection expression seeming to be unsupported. but why my visual studio allows that...

[NiklasGustafsson]

oh.. that is the collection expression seeming to be unsupported. but why my visual studio allows that...

Yeah, we need to support both .NET FX 4.7.2 and .NET 6.0, so you have to stick to some lowest common denominators.

[yueyinqiu]

And what about the build failure that still exists in Windows_x64_NetFX? Hope that it's not introduced by me, since #1261 is also facing that, which haven't touch any code file at all.

[NiklasGustafsson]

And what about the build failure that still exists in Windows_x64_NetFX? Hope that it's not introduced by me, since #1261 is also facing that, which haven't touch any code file at all.

That one is not your fault. I have a fix coming for that -- we updated the reference to ImageSharper in one of the examples, and it no longer supports .NET FX. That breakage will not block the merge of this PR.