added dspData struct and appropriate get_dsp() -methods to work with it. #31
Conversation
There was a problem hiding this comment.
I really like this!
However, there's some typing that isn't quite correct, and there's the addition of the new filesystem implementation that I don't think will compile as-is?
Since those might cause problems, it would probably be easiest to decrease the scope of this PR to defining dspData and extending the get_dsp() interface.
| @@ -1,6 +1,6 @@ | |||
| #pragma once | |||
|
|
|||
| #include <filesystem> | |||
There was a problem hiding this comment.
This is really nice having all these #includes removed :)
| @@ -22,6 +25,8 @@ enum EArchitectures | |||
| kNumModels | |||
| }; | |||
|
|
|||
| using sample = float; | |||
There was a problem hiding this comment.
What's the scope of this? It'd be global, wouldn't it? Since it's in a header?
The NAM models process floats, but the other DSP (the EQ filters) might operate on doubles.
I know it's a bit messy right now, but this is raising a flag for me about potential problems.
| @@ -69,7 +74,7 @@ class DSP | |||
| // Should we normalize according to this loudness? | |||
| bool mNormalizeOutputLoudness; | |||
| // Parameters (aka "knobs") | |||
| std::unordered_map<std::string, double> _params; | |||
| std::unordered_map<std::string, sample> _params; | |||
There was a problem hiding this comment.
sample isn't a good type for params. Ideally, they'd have their own type.
That type would be float though, based on how the training currently is set up (using torch.Tensors, which are single-precision by default).
| std::string architecture; | ||
| nlohmann::json config; | ||
| nlohmann::json metadata; | ||
| std::vector<float> params; |
There was a problem hiding this comment.
Notice floats here vs line 72. Not good.
| @@ -191,11 +196,24 @@ class Conv1x1 | |||
| // Utilities ================================================================== | |||
| // Implemented in get_dsp.cpp | |||
|
|
|||
| struct dspData | |||
| @@ -3,12 +3,15 @@ | |||
| #include <stdexcept> | |||
| #include <unordered_set> | |||
|
|
|||
| #include "ghc/fs_std_impl.hpp" | |||
There was a problem hiding this comment.
What's this? Looks like a filesystem implementation?
I think i like it in principle, but I don't see it with this PR? I'd prefer overall to have this change (filesystem tools) in a separate PR.
|
Resolves #45. |
|
Sigh! Somehow I thought that new pushes on this branch would not propagate to the pull request automatically... Little did I know. I will, once again, create a new pull request with just the dspStruct addition 🙃 |
Let's try this again. :) Just the dspData struct addition, I added the metadata as a JSON as I don't know the exact properties (and I guess they are still on poll anyways?) but we can always add more specific data in in the future I guess.