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

Remove parametric model code #81

Closed
mikeoliphant opened this issue Oct 7, 2023 · 5 comments · Fixed by #95
Closed

Remove parametric model code #81

mikeoliphant opened this issue Oct 7, 2023 · 5 comments · Fixed by #95
Assignees

Comments

@mikeoliphant
Copy link
Contributor

Thought I'd add an issue to track this.

Looking at the code, it seems like if we get rid of the parametric stuff from WaveNet, we can get rid of the "condition" matrix, avoid a copy, and work directly on the input:

// Fill into condition array:
// Clumsy...
for (int j = 0; j < _num_input_samples; j++)
{
this->_condition(0, j) = _input_samples[j];
if (this->_stale_params) // Column-major assignment; good for Eigen. Let the
// compiler optimize this.
for (size_t i = 0; i < this->_param_names.size(); i++)
this->_condition(i + 1, j) = (float)this->_params[this->_param_names[i]];
}

@mikeoliphant
Copy link
Contributor Author

Thought about this a bit more, and I guess we'd still need to copy into condition if NAM_SAMPLE is double...

Might be worth considering making the model process float-only.

@sdatkinson
Copy link
Owner

All good points.

I do want to leave the door open for downstream adaptations of this library to be able to do conditional models. I'm not looking super-closely at it right now, but some function that "prepares" the input? Here, the implementation might be to make a 1-by-nsamples Eigen matrix that references the data (and avoid a copy; I haven't checked closely but maybe #73 is about this? Eigen's documentation website doesn't seem to be working right now...which is also kinda worrisome! but); the parametric implementation can override this and get its (1+nconditions)-by-nsamples array, and the rest downstream processing can be happy working with the info from the matrix it's given?

But I think I agree about the double-versus-float bit, especially since this library is focusing on neural effects, which I don't expect to use double precision.

@mikeoliphant
Copy link
Contributor Author

mikeoliphant commented Oct 15, 2023

I think Eigen's "Map" might be what we want here - hard to check for sure right now, though, since the Eigen docs are offline. It looks like it might let you map an array into a matrix without copying.

@mikeoliphant
Copy link
Contributor Author

mikeoliphant commented Oct 15, 2023

For handling parameters, it seems like the use case is going to be a small number of parameters that often don't change (ie: a gain knob). Maybe some sort of "set_param" method?

We can maybe ignore the details now as long as we still pass a condition matrix to the underlying layer array processing. A parameterized implementation could then override "process()" and handing mapping in the parameters.

@sdatkinson
Copy link
Owner

Yep, both sound great :)

And I can always make tweaks to the implementation of process() anyways if a more granular decomposition of what's going on makes it easier to slide that extension in, no worries.

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 a pull request may close this issue.

2 participants