search

jatinchowdhury18 / RTNeural

Real-time neural network inferencing
BSD 3-Clause "New" or "Revised" License
543 stars 57 forks source link

Implement grouped convolutions #116

Closed purefunctor closed 7 months ago

purefunctor commented 7 months ago

Closes #114. This adds the groups_of parameter and an alternative implementation for forward that works on grouped convolutions. I had to conditionally define forward depending on the value of groups_of since I had trouble reconciling the implementation with groups_of=1

purefunctor commented 7 months ago

TODO:

jatinchowdhury18 commented 7 months ago

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.

purefunctor commented 7 months ago

I also pushed my attempt at implementing microTCN 9d6545c but inference isn't correct yet. Do you reckon there's something I'm missing?

codecov-commenter commented 7 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (37fde44) 95.64% compared to head (7aebcac) 95.70%. Report is 2 commits behind head on main.

Files Patch % Lines
RTNeural/model_loader.h 71.42% 2 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jatinchowdhury18 commented 7 months ago

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 :).

jatinchowdhury18 commented 7 months ago

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.

jatinchowdhury18 commented 7 months ago

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.

purefunctor commented 7 months ago

I can merge your branch into this one so it's easier to do the merge into main 👍🏼

jatinchowdhury18 commented 7 months ago

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!

jatinchowdhury18 commented 7 months ago

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!