Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/add test pytorch imported models #79

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

MaxPayne86
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Dec 22, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.63%. Comparing base (74d813d) to head (c3d20a0).
Report is 64 commits behind head on main.

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@jatinchowdhury18 jatinchowdhury18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is looking great!

Would it be possible to also add in the scripts that you used to generate the .json and .csv files? That can be a helpful reference in case we want to expand our tests in the future. It would be nice if the scripts were as minimal as possible, but in case some more dependencies are needed, then maybe adding some comments about how to get the required dependencies would be enough.

CMakeLists.txt Outdated
@@ -1,5 +1,6 @@
cmake_minimum_required(VERSION 3.1)
project(RTNeural VERSION 1.0.0)
set(CMAKE_CXX_STANDARD 17)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RTNeural is intended to be compatible with C++14, so we shouldn't be setting this in the top-level CMakeList.

const std::string pytorch_tf_model = "models/pytorch_imported.json";
const std::string pytorch_x = "test_data/pytorch_x.csv";
const std::string pytorch_y = "test_data/pytorch_y.csv";
constexpr double threshold = 1.0e-12;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@MaxPayne86
Copy link
Contributor Author

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.

@jatinchowdhury18
Copy link
Owner

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

@MaxPayne86
Copy link
Contributor Author

@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!

@jatinchowdhury18
Copy link
Owner

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants