SigMF icon indicating copy to clipboard operation
SigMF copied to clipboard

first implementation of wav2sigmf.py

Open gmabey opened this issue 2 years ago • 15 comments

In order to see what works and what doesn't, I suggest you run this:

#!/usr/bin/env python3

import os, glob
import scipy.io

data_dir = os.path.join(os.path.dirname(scipy.io.__file__), 'tests', 'data')
for wf in glob.glob(data_dir + '/*.wav'):
    os.system('converters/wav2sigmf.py ' + wf)

gmabey avatar Jan 18 '22 19:01 gmabey

I'm ok with this PR taking a while to percolate, but I kinda would appreciate hearing whether there is general interest in the project accepting and maintain a battery of converter scripts/functions to facilitate coming over to the SigMF side.

gmabey avatar Jan 28 '22 02:01 gmabey

I have thought about this a few times, and am leaning against having converters like this within this repository primarily because of the natural tendency for tools like this to become an unbounded source of maintenance. E.g.: "we have wav, lets add support for flac... pcm... mp3.." and starts to pull us away from the original mission.

Id like to hear the opinions of other maintainers though @bhilburn @777arc @Teque5.

jacobagilbert avatar Jan 28 '22 04:01 jacobagilbert

I would take what we can get as long as it doesn't messy up the rest of the code or cause increased maintenance in the long term, e.g., if we anticipate having to update this wav2sigmf.py in the future as part of changes to the standard, but I don't think that will be the case because backwards compatibility is pretty critical with this kind of standard. If someone else comes along and creates flac/pcm/mp3 converters, that's great, as long as it doesn't use a ton of the rest of the maintainer's time. I think if anything the discussion should be more about whether or not we want a "converters" dir in the root =P

777arc avatar Jan 28 '22 05:01 777arc

Yea I think this kind of tool is pretty useful. If it uses the intended entry points for sigmf then it should be maintainable.

If we decide to accept this PR, I can modify it to add an entry point in the setup.py such that this gets installed correctly and if you install it gets related dependencies (getpass) as an optional. If I did that then generally this code would get ignored unless you did pip install sigmf[convert].

Teque5 avatar Jan 29 '22 04:01 Teque5

Maybe @gmabey will give you write access to his fork to make those additions 😃

777arc avatar Jan 29 '22 04:01 777arc

Ok there is sufficient support and I my initial position was very weak. Let's get these changes made and bring it in. In terms of code structure, I think it might be better to have this in an "examples" dir as opposed to "converters".

Thoughts?

jacobagilbert avatar Jan 29 '22 16:01 jacobagilbert

I think I still like converters/ even though you're right they would simultaneously be examples.

When I hear the word "examples" I think it implies in my mind that it doesn't actually do anything useful.

gmabey avatar Jan 29 '22 16:01 gmabey

Maybe @gmabey will give you write access to his fork to make those additions

I'm happy to accept and review PR's to my fork

gmabey avatar Jan 29 '22 16:01 gmabey

I think I still like converters/ even though you're right they would simultaneously be examples.

When I hear the word "examples" I think it implies in my mind that it doesn't actually do anything useful.

Examples that are not useful will get the 🧹

I am suggesting a more broad directory as the SigMF project is invested in building more examples and this is a god one. It also takes the pressure and expectation away from the effort providing comprehensive conversion tools.

@777arc you had a similar comment above thoughs?

jacobagilbert avatar Jan 29 '22 16:01 jacobagilbert

It's also possible this belongs in sigmf/examples or converters or tools or whatever we choose to be distributed with the module.

@bhilburn should we require similar DCO signoff as GR does? This is a significant contribution.

jacobagilbert avatar Jan 29 '22 16:01 jacobagilbert

Examples that are not useful will get the 🧹

In my mind, there is a significant difference between "sample code" (to draw a C++ analogy, code that demonstrates how to properly invoke a function) and a "utility" (like, a compiled application that performs a specific operation and does it reliably). In the spirit of encouraging others to adopt SigMF as a file format (and leave behind others), I suggest we aim towards the latter.

I am suggesting a more broad directory as the SigMF project is invested in building more examples and this is a god one. It also takes the pressure and expectation away from the effort providing comprehensive conversion tools.

Hmm, you want to avoid the impression that wav2sigmf.py will work comprehensively? Or, that there exists a comprehensive set of converters? Maybe that is appropriate, given the cases in which the sample script I provided fails, like, yes maybe "comprehensive" should not be the criterion/aim. I think that "useful" and "reliable to the point of clearly indicating when it fails" should be the goal for a project-maintained set of utilities that have been tested (by the community) as converting to the SigMF file format.

gmabey avatar Jan 29 '22 17:01 gmabey

Utils or tools are also highly reasonable. At the end of the day this is a fairly specific tool but it has the widespread benefit of showing how the module can be used in a simple and straightforward way.

My goal is to ensure the project can retain adequate focus on the development of the specification. There are myriad converters we could include and should not. Our tooling already has issues with bitrot as you have identified.

Thinking more, this definitely needs go inside sigmf/ it's relevant to the module specifically. Should also be included in the package.

jacobagilbert avatar Jan 29 '22 17:01 jacobagilbert

I like /examples or /utils or /tools over /converters because there will be non-converter things we add and we aren't trying to collect an enormous list of converters. Yeah definitely inside sigmf/ and at some point the python module may get put into its own repo anyway and this might as well go along with it.

777arc avatar Jan 31 '22 05:01 777arc

I did move wav2sigmf.py to sigmf/tools/ (utils/ wouldn't work as there's already a utils.py in there.

I think that this file is unique (in SigMF) in that it's intended to be invoked as a command-line application, but is also useful for importing in other code. I don't know what you think about its current form, but I like it, and I think the only additional thing would be to get it on the path somehow. I would very much appreciate any tips on how to do this, but if this PR is accepted, I suppose that could be added afterwards as well.

gmabey avatar Feb 13 '22 05:02 gmabey

LGTM

777arc avatar Feb 13 '22 06:02 777arc