RTNeural icon indicating copy to clipboard operation
RTNeural copied to clipboard

Feature/add test pytorch imported models

Open MaxPayne86 opened this issue 2 years ago • 5 comments

MaxPayne86 avatar Dec 22 '22 16:12 MaxPayne86

Codecov Report

Merging #79 (c3d20a0) into main (74d813d) will decrease coverage by 0.08%. The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #79      +/-   ##
==========================================
- Coverage   96.72%   96.63%   -0.09%     
==========================================
  Files          29       29              
  Lines        2318     2437     +119     
==========================================
+ Hits         2242     2355     +113     
- Misses         76       82       +6     
Impacted Files Coverage Δ
RTNeural/activation/activation.h 91.13% <0.00%> (-7.35%) :arrow_down:
RTNeural/conv1d/conv1d_eigen.h 92.85% <0.00%> (-0.48%) :arrow_down:
RTNeural/gru/gru_xsimd.h 97.77% <0.00%> (-0.05%) :arrow_down:
RTNeural/lstm/lstm_xsimd.h 98.00% <0.00%> (-0.04%) :arrow_down:
RTNeural/gru/gru.tpp 100.00% <0.00%> (ø)
RTNeural/lstm/lstm.tpp 100.00% <0.00%> (ø)
RTNeural/model_loader.h 86.80% <0.00%> (ø)
RTNeural/conv1d/conv1d.tpp 100.00% <0.00%> (ø)
RTNeural/gru/gru_xsimd.tpp 100.00% <0.00%> (ø)
RTNeural/lstm/lstm_xsimd.tpp 100.00% <0.00%> (ø)
... and 13 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov-commenter avatar Dec 22 '22 21:12 codecov-commenter

Would it be possible to also add in the scripts that you used to generate the .json and .csv files?

Script is here. Problem is that currently a CoreAudioML submodule would be needed at least since:

import CoreAudioML.miscfuncs as miscfuncs
import CoreAudioML.networks as networks
import CoreAudioML.dataset as dataset

Let me know if it's okay. Refactoring the code to avoid these deps would probably divert from what we want to do.

RTNeural is intended to be compatible with C++14

Yup, committed by mistake, reverted.

MaxPayne86 avatar Dec 23 '22 08:12 MaxPayne86

Sure, having the CoreAudioML imports in there is fine, although I guess it would be good to add a comment at the top of the script like # This script depends on the CoreAudioML library (https://github.com/link/to/the/repo).

jatinchowdhury18 avatar Dec 23 '22 17:12 jatinchowdhury18

@jatinchowdhury18 hello I would like to discuss this PR: I've seen that RTNeural introduces apis for importing pytorch models. On my end I've embedded engine validation inside the plugin. Btw if you think it's still cool to have it I can provide the missing files so that we can merge. Thanks a lot!

MaxPayne86 avatar May 17 '23 08:05 MaxPayne86

@MaxPayne86 Sure thing! My idea with some of the more recent commits was to add an interface that could be used to load PyTorch models directly from the state_dict JSON representation. I think having the script that could be used to convert from a PyTorch-style JSON file to an RTNeural-style JSON file wouldn't directly conflict with that, but I do worry that it might be a little bit confusing having two different ways of solving the same problem. It's probably more of a documentation problem that I would need to solve on my end, but if you have some ideas that would be very helpful as well!

But anyway, for finishing up this PR, I think the steps would be:

  • Rebase off of the latest changes to main
  • Add the missing scripts
  • Update the tests to use the new built-in methods for loading PyTorch layers (so we don't have copies of transpose() defined in multiple places)

That would be fantastic, and thanks again for all your work on this!

jatinchowdhury18 avatar May 18 '23 15:05 jatinchowdhury18