Implement grouped convolutions #116
Conversation
|
TODO:
|
|
Nice! I haven't had time yet to go through the PR in detail, but at first glance, everything looks great! I wouldn't worry about the other backends just yet, and I'm happy to help with those implementation as well. |
There was a problem hiding this comment.
Looking good! I did a little bit of work on another branch mostly just rebasing from main and messing with the test code a little bit. I think this weekend I can go through and work more on the other implementations.
| jsonStream >> modelJson; | ||
|
|
||
| RTNeural::ModelT<T, 6, 3, RTNeural::Conv1DT<T, 6, 3, 1, 1, 3, false>> model; | ||
| RTNeural::torch_helpers::loadConv1D<T>(modelJson, "", model.template get<0>()); |
There was a problem hiding this comment.
Do you know if TensorFlow also has a "groups" concept for their Conv1D layer? That might be a good thing to test for also, although we can add that in a future PR if it turns out to be complicated.
There was a problem hiding this comment.
I believe so, and it's functionally the same as PyTorch's
There was a problem hiding this comment.
Okay cool! I can take a look at adding a test case on the Tensorflow side.
|
I also pushed my attempt at implementing microTCN 9d6545c but inference isn't correct yet. Do you reckon there's something I'm missing? |
This definition makes it so that an apparent off-by-one error is fixed by instead making the `start` index be the difference between the target and the current length. For example, given a tensor with 1000 elements, and a target of 970, the function would instead crop with `x[..., 30:]`, rather than the previous behaviour of `x[..., 29:999]`. This seems to be more correct in that the previous behaviour takes items from index 29 (inclusive) to index 999 (exclusive, so 998). Meanwhile, the new behaviour makes it so that it's index 30 (inclusive) until the end, which is the index 999.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #116 +/- ##
==========================================
+ Coverage 95.64% 95.70% +0.06%
==========================================
Files 58 58
Lines 3860 3891 +31
==========================================
+ Hits 3692 3724 +32
+ Misses 168 167 -1 ☔ View full report in Codecov by Sentry. |
|
Nice work on getting the TCN working! I think I have the necessary changes for the non-templated STL implementation and other backends more or less ready to go, but I wanted to ask what you think is the best way to get those changes merged into your branch. I could continue working on this branch, but it's probably a bit out of date since your latest changes on this branch. By the way, I think I'm also going to re-write the unit tests in Catch2 or something this weekend, so hopefully we can get this PR merged-in before then :). |
|
Sorry, just getting back to this today. On my branch I've added:
Hopefully I can finish up the Eigen implementations tonight or tomorrow, and then we should be good to merge! I'd like to get the MicroTCN test case working for all the backends as well, but I'm okay with doing that after we merge this PR depending on when I get around to it. |
|
Update: The Eigen implementation is now working as well, so the only work left to do should be some documentation and to finish up the MicroTCN test. That said, I think I'm going to wait until #119 is merged before merging these changes, just to avoid some tricky merge conflicts. |
|
I can merge your branch into this one so it's easier to do the merge into main 👍🏼 |
This definition makes it so that an apparent off-by-one error is fixed by instead making the `start` index be the difference between the target and the current length. For example, given a tensor with 1000 elements, and a target of 970, the function would instead crop with `x[..., 30:]`, rather than the previous behaviour of `x[..., 29:999]`. This seems to be more correct in that the previous behaviour takes items from index 29 (inclusive) to index 999 (exclusive, so 998). Meanwhile, the new behaviour makes it so that it's index 30 (inclusive) until the end, which is the index 999.
|
Alrighty! The other branch should be ready to merge, if you'd like to pull those changes into this PR. Also feel free to add yourself to the contributors list if you'd like! |
…ups' into pure/implement-groups
|
Looks like everything is good to go except for formatting. I'll go ahead and merge, and I can take care of formatting later. Thanks again for your work on this! |
jatinchowdhury18
merged commit 1e81449
into
jatinchowdhury18:main
Closes #114. This adds the
groups_ofparameter and an alternative implementation forforwardthat works on grouped convolutions. I had to conditionally defineforwarddepending on the value ofgroups_ofsince I had trouble reconciling the implementation withgroups_of=1