Skip to content

Allow NAM_SAMPLE_FLOAT to switch model input to float instead of double#48

Merged
sdatkinson merged 2 commits into
sdatkinson:mainfrom
mikeoliphant:nam_sample
Jun 23, 2023
Merged

Allow NAM_SAMPLE_FLOAT to switch model input to float instead of double#48
sdatkinson merged 2 commits into
sdatkinson:mainfrom
mikeoliphant:nam_sample

Conversation

@mikeoliphant

Copy link
Copy Markdown
Contributor

This PR adds a NAM_SAMPLE #define that is used instead of "double" for input samples for model processing.

If NAM_SAMPLE_FLOAT is defined, this will switch NAM_SAMPLE to "float". Otherwise it remains "double", so no there is no change in existing behavior.

This allows processing loops using "float" to avoid having to convert buffers to "double".

@daleonov

daleonov commented Jun 9, 2023

Copy link
Copy Markdown
Contributor

Good stuff. JUCE, for instance, uses float buffers, so keeping an extra double-precision buffer as an adapter for NAM is a bit annoying.

@sdatkinson sdatkinson left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Approving since I've called it a nit, but I'd appreciate hearing back from you about having gain parameters be of NAM_SAMPLE type. It feels odd, but I know I haven't seen enough DSP code to feel confident about conventions in the space.

Comment thread NAM/convnet.cpp Outdated
}

convnet::ConvNet::ConvNet(const double loudness, const int channels, const std::vector<int>& dilations,
convnet::ConvNet::ConvNet(const NAM_SAMPLE loudness, const int channels, const std::vector<int>& dilations,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Nit: I don't like loudness being of "sample" type--it's a parameter of the model.

But I get that it mainly serves to do arithmetic with samples, so I can live with it.

Comment thread NAM/dsp.cpp Outdated
void DSP::process(double** inputs, double** outputs, const int num_channels, const int num_frames,
const double input_gain, const double output_gain,
void DSP::process(NAM_SAMPLE** inputs, NAM_SAMPLE** outputs, const int num_channels, const int num_frames,
const NAM_SAMPLE input_gain, const NAM_SAMPLE output_gain,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yeah, it looks weird having all of these parameters being called "samples". Can you point me to somewhere else that follows this convention? It just doesn't sound right.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure either, and I'm fine either way. "loudness" is an internal parameter anyway, and "input_gain" and "output_gain" should be probably be removed since the plugin just passes "1.0" and handles gain itself (as does my LV2 plugin).

@daleonov

Copy link
Copy Markdown
Contributor

By the way, what about Eigen::MatrixXf and Eigen::VectorXf? Shouldn't they match buffer precision instead of always being float?

@mikeoliphant

Copy link
Copy Markdown
Contributor Author

By the way, what about Eigen::MatrixXf and Eigen::VectorXf? Shouldn't they match buffer precision instead of always being float?

No - the models operate with floats regardless of the input sample precision.

@mikeoliphant

Copy link
Copy Markdown
Contributor Author

Ok, I've reverted the parameters to double.

@sdatkinson sdatkinson left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM! 👍🏻

@sdatkinson sdatkinson merged commit e14c50b into sdatkinson:main Jun 23, 2023
@mikeoliphant mikeoliphant deleted the nam_sample branch July 28, 2023 16:11
synchu pushed a commit to synchu/NeuralAmpModelerCore that referenced this pull request Mar 24, 2026
* More error codes related for loading WAV files

* Better error messages

* Add enum of return codes and capture IR loading in IR state

* Error messages for failed IR loads.
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