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.

Sign up for GitHub

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

Jump to bottom

Make sure reconstruction test is runnable end to end #16

Open
danpovey opened this issue Oct 22, 2019 · 8 comments
Open

Make sure reconstruction test is runnable end to end #16

danpovey opened this issue Oct 22, 2019 · 8 comments

Comments

@danpovey
Copy link
Owner

@soroushzargar perhaps you could do this.. it would be better if the reconstruction-test were easier to run. For example, have a script that downloads and suitably formats the appropriate data and then runs the test. Right now there is some hacky stuff going on with a temp dir that probably isn't in the source tarball.

I was only printing out the bits in the file as a debugging step. But I think we should be displaying the file sizes or bits per unit time in some form alongside the various compression methods; this will make comparisons more meaningful.

I just set the bits per sample to 6 in reconstruction-test.py but we may want to test various settings. What I would really like is some info about PSNR's and file sizes formatted in a way that's easily readable. IMO a simple Python program that prints out a summary would be preferable to this CSV stuff, in that it would be clearer. But as long as you do print out something readable at some point (maybe as a post processing step that might be in a separate script) it's OK with me. Make sure it's all documented with a top-level run.sh or something.
@soroushzargar
Copy link
Collaborator

@soroushzargar perhaps you could do this.. it would be better if the reconstruction-test were easier to run. For example, have a script that downloads and suitably formats the appropriate data and then runs the test. Right now there is some hacky stuff going on with a temp dir that probably isn't in the source tarball.

Ok I think a better way is that the file itself should be a runnable file with passing arguments; something like

`./reconstruction-test --dataset-dir --sample-rate

arguments passed by should be

  • --dataset-dir: An address to a dataset directory which includes some .wav files.
  • --sample-rate: The sample rate in which the comparison is supposed to work with.
  • --release-df: If this flag is appeared, then the procedure will create a set of pandas dataframes saved in .csv format which could be used with pandas later.
  • --release-log It prints out a tab delimited log file which will show the output of comparison.

This will shorten the code and also reduces dependancies.

I was only printing out the bits in the file as a debugging step. But I think we should be displaying the file sizes or bits per unit time in some form alongside the various compression methods; this will make comparisons more meaningful.

The output of the file can be in following shape

Results on sampling rate = <sampling rate>
<Filename>    <algorithm>   <bitrate>   <psnr> [repeating for each compression algorithm]

I just set the bits per sample to 6 in reconstruction-test.py

By that you mean 16? Sorry here I just got confused.

but we may want to test various settings. What I would really like is some info about PSNR's and file sizes formatted in a way that's easily readable.

Did I cover that in previous part?

IMO a simple Python program that prints out a summary would be preferable to this CSV stuff, in that it would be clearer. But as long as you do print out something readable at some point (maybe as a post processing step that might be in a separate script) it's OK with me. Make sure it's all documented with a top-level run.sh or something.

Ok I think a top level .sh with man documentation is what you want here. Right?

@danpovey
Copy link
Owner Author

`./reconstruction-test --dataset-dir --sample-rate

Good idea.. but be sure to have a top-level run.sh that runs the complete test.

arguments passed by should be

  • --dataset-dir: An address to a dataset directory which includes some .wav files.
  • --sample-rate: The sample rate in which the comparison is supposed to work with.
  • --release-df: If this flag is appeared, then the procedure will create a set of pandas dataframes saved in .csv format which could be used with pandas later.
  • --release-log It prints out a tab delimited log file which will show the output of comparison.

Sure.

I just set the bits per sample to 6 in reconstruction-test.py

By that you mean 16? Sorry here I just got confused.

No I meant 6- look at the code. The bits-per-sample used by lilcom to compress.

but we may want to test various settings. What I would really like is some info about PSNR's and file sizes formatted in a way that's easily readable.

Did I cover that in previous part?
Yes, I think so.

IMO a simple Python program that prints out a summary would be preferable to this CSV stuff, in that it would be clearer. But as long as you do print out something readable at some point (maybe as a post processing step that might be in a separate script) it's OK with me. Make sure it's all documented with a top-level run.sh or something.

Ok I think a top level .sh with man documentation is what you want here. Right?

Yes, a top-level sh. Just put comments in where needed. man documentation is super overkill for a test script.

@soroushzargar
Copy link
Collaborator

`./reconstruction-test --dataset-dir --sample-rate

Good idea.. but be sure to have a top-level run.sh that runs the complete test.

We can make the python as a runnable shell file just with ! /bin/python or sth, but I think you want sth more than just adding this and saving as a sh file. Can you say what you expect from the sh file which this additional line can not do?

I just set the bits per sample to 6 in reconstruction-test.py

By that you mean 16? Sorry here I just got confused.
No I meant 6- look at the code. The bits-per-sample used by lilcom to compress.

Yes found it. we can add another passing parameter to let the user change this value. Is it Ok or sth else is needed.

Ok I think a top level .sh with man documentation is what you want here. Right?

Yes, a top-level sh. Just put comments in where needed. man documentation is super overkill for a test script.

The argument parser gives a help function in which the user can easily find out how to work with the test.

@danpovey
Copy link
Owner Author

The idea of run.sh is that it should just run automatically when you run
./run.sh
so the user does not have to spend time figuring out documentation.

@soroushzargar
Copy link
Collaborator

soroushzargar commented Oct 25, 2019 via email

@soroushzargar
Copy link
Collaborator

In addition to the assumption that MP3 will compress with the exact bitrate passed also there would be an issue with sample rate. Let me test it on long audio too and I'll return the code with true bitrates for mp3.

Do you think we should substitute pydub and librosa with sox. If it is necessary I try to figure it out.

@danpovey
Copy link
Owner Author

danpovey commented Nov 11, 2019 via email

@danpovey
Copy link
Owner Author

danpovey commented Nov 11, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@danpovey @soroushzargar