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

v1.4.0 build api #106

Closed
rabitt opened this issue May 21, 2020 · 6 comments
Closed

v1.4.0 build api #106

rabitt opened this issue May 21, 2020 · 6 comments
Labels

Comments

@rabitt
Copy link
Collaborator

rabitt commented May 21, 2020

So far, in 1.4.0a0, we've overloaded the build API to support 4 cases:

  1. file in, file out
    status = tfm.build(input_filepath=..., output_filepath=...)
  2. file in, array out
    status, out, err = tfm.build(input_filepath=...)
  3. array in, file out
    status = tfm.build(input_array=..., sample_rate_in=..., output_filepath=...)
  4. array in, array out
    status, out, err = tfm.build(input_array=..., sample_rate_in=...)

The pros:

  • it's backwards compatible
  • it's all in one function

The cons:

  • the returns change in a confusing way as a function of the combination of inputs
  • only certain combinations of inputs are valid

Do we want to keep it this way? Some other alternatives:
A. write 4 separate functions. We keep build as file in-file out for backwards compatiblity and define 3 new functions for the other cases
B. write 2 separate functions, one for file output and one for array output, keeping the input argument logic (e.g. build and build_array, similar to @pseeth 's original PR) in order to support either file or array input for both functions
C. keep build as it is currently, and write 4 additional wrapper functions for each of the 4 cases that internally call build
... other ideas?

Would love to hear people's thoughts.
cc @pseeth @nicolamontecchio @psobot @carlthome @hadware @lostanlen

@pseeth
Copy link
Contributor

pseeth commented May 21, 2020

I overall think it's fine. I really prefer having one access point to using pysox - the build function. The perhaps more worrying thing is that the Combiner does not fit the same build API, but they are I guess different classes so maybe it's okay? If anyone has a way to make the Combiner also take numpy arrays (e.g. multiple input arrays, or combination of arrays and files?), then that'd be great. We also have numpy now so we could (perhaps by adding soundfile as a dependency) do combining in-memory instead of using up an exec call for summing stuff.

@hadware
Copy link
Contributor

hadware commented May 21, 2020

I've looked a bit into the Combiner issue when I did a bit of documenting to work on this issue, and IIRC, there is no way with sox to use multiple piped inputs for processing (and thus combining). I'm wondering if it's possible to use a combiner on 1 piped array and multiple other file inputs.

I think the build and build_array solution (i.e. solution B) is the best, as it clearly outlines what is being returned by the function, without having to dig into the function's arguments documentation (and somewhat reduces the number of arguments in both functions). Moreover, it has a minimal impact on the original build function's signature.

@pseeth
Copy link
Contributor

pseeth commented May 21, 2020

We could possibly do both by adding some convenience functions that call build in different ways? For example, build_array_to_array, build_file_to_file (maybe this one could become build to maintain backwards compatibility), build_file_to_array, build_array_to_file. These would all use the existing build function, which would get renamed to _build.

@rabitt
Copy link
Collaborator Author

rabitt commented May 22, 2020

We could possibly do both by adding some convenience functions that call build in different ways? For example, build_array_to_array, build_file_to_file (maybe this one could become build to maintain backwards compatibility), build_file_to_array, build_array_to_file. These would all use the existing build function, which would get renamed to _build.

@pseeth yeah this is what I had in mind with option C. We'd need to keep build backward compatible though.

I'm currently leaning towards "B" but I'll need to see how it looks under the hood.

@justinsalamon
Copy link
Contributor

👀

@rabitt
Copy link
Collaborator Author

rabitt commented Jun 2, 2020

Closed in #107

@rabitt rabitt closed this as completed Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants