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

Use FileIO mechanism for IO #30

Open
nicolamos opened this issue Oct 24, 2022 · 6 comments
Open

Use FileIO mechanism for IO #30

nicolamos opened this issue Oct 24, 2022 · 6 comments
Assignees

Comments

@nicolamos
Copy link

I found that the dependency on WAV.jl leads to inconsistent behaviour when using this package with others (specifically in the JuliaAudio ecosystem).

I think it should be better to rely on FileIO without depending on WAV.jl directly. The user should be able to select the backend.
I don’t know if this is possible without much effort (the underlying representation can change vastly). Another way around is to drop support for IO, giving the user good explanation on how to load a signal from an audio file.

The problem is that if one has both WAV.jl and LibSndFile.jl in the same manifest, FileIO would prefer WAV.jl anyway.

@mchitre
Copy link
Member

mchitre commented Oct 24, 2022

I just took a look at FileIO integration with WAV and LibSndFile. The returned data is different in both cases, so they are not drop-in replacements for each other. While SignalAnalysis could easily support both via FileIO, when FileIO adds other backends, SignalAnalysis will need to change. The correct way to do this would be for FileIO to really support both backends without changing returned data format. Without that, the backend used is not transparent to the user of FileIO.

@mchitre mchitre self-assigned this Oct 24, 2022
@mchitre mchitre added the enhancement New feature or request label Oct 24, 2022
@nicolamos
Copy link
Author

To be fair, I do not think that is FileIO’s responsibility to provide a consistent representation of the data. It has the notion of File and FileFormat, providing a general interface for dealing with IO. While it is understandable that there could be many backends with different representations, it is responsibility of the ecosystem to provide a consistent API and data representation. In this regard, we can look at JuliaImages: an image can be anything that is array-like, as a plain Array, an AxisArray, or a MetaArray. IO operations leverage ImageIO, which provides a bridge between FileIO and various packages to which the actual IO operations are delegated.

My point is that, ideally, there should be an AudioIO.jl package in the JuliaAudio ecosystem offering such a bridge, provided there is also a standardized signal format in Julia. In the meantime, I think that there is no need to change SignalAnalysis much, as it is not a package devoted to IO. What could be done is to provide conversion functions between, e.g. SampledSignals.jl data types and the current representation, leaving the user to deal with more exotic representations.

I am willing to contribute, if necessary.

@mchitre
Copy link
Member

mchitre commented Oct 25, 2022

If you have a concrete suggestion on how you'd like to see SignalAnalysis.jl changed, I'm open to a PR.

I'd like to avoid a breaking change where the support for signal(filename) is dropped, as far as possible. If there is a way to support both backends without explicitly add the dependency on LibSndFile.jl (and perhaps reducing the dependency on WAV.jl), that would be nice.

@nicolamos
Copy link
Author

Depending on your plans for the package, there are some options that come to my mind. You can surely provide a better insight on the details of the package as I didn’t look much into it.

The first thing I would do is to rely on FileIO, dropping the direct dependency on WAV.jl. If the user needs it, can always add it to its local environment.

Then we have some options to make signal(filename) work, more or less. None of them requires a direct dependency on LibSndFile.jl nor WAV.jl.

  • One possibility is to add SampledSignals so one can dispatch on the type to convert to the representation of SignalAnalysis; but this requires a rather huge dependency;
  • Another one is simply to provide the user with an error if the type is not a tuple (what WAV.jl) currently returns. In that case the user can use, perhaps, signal(array, fs), since SampledSignals.SampleBuf is an AbstractArray and is the type used in LibSndFile.jl; in this way you do not need to depend explicitly on SampledSignals.

@nicolamos
Copy link
Author

nicolamos commented Oct 26, 2022

Another possibility that is viable (and probably not orthogonal to the other ones) (just look for reference at SignalOperators.jl for reference) is to lazy load dependencies with Requires.jl. In this way, of course, the package will need to change as more backends need to be supported.

@mchitre
Copy link
Member

mchitre commented Oct 29, 2022

Rather than add Requires for each package, depending on FileIO and dealing with different return types may be a better option.

Another one is simply to provide the user with an error if the type is not a tuple (what WAV.jl) currently returns. In that case the user can use, perhaps, signal(array, fs), since SampledSignals.SampleBuf is an AbstractArray and is the type used in LibSndFile.jl; in this way you do not need to depend explicitly on SampledSignals.

This is my preferred choice, perhaps with a friendly error message telling the user what needs to get done if the user isn't using WAV.jl as the backend.

@mchitre mchitre removed the enhancement New feature or request label Nov 1, 2024
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

No branches or pull requests

2 participants