search

danpovey / lilcom

Small compression utility
Other
33 stars 10 forks source link

Reconstruction test #20

Closed soroushzargar closed 5 years ago

soroushzargar commented 5 years ago

This is a more modular and well structured re-written version of the reconstruction test. There are also some more modifications I should do which I will in a few days. I should reduce its dependencies and some other modifications. But the current code is a way better than what I wrote previous time.

soroushzargar commented 5 years ago

Yes in the branch I totally removed the previous file and wrote the code again.

On Sat, 26 Oct 2019, 01:20 Daniel Povey, notifications@github.com wrote:

@danpovey commented on this pull request.

In test/test_reconstruction.py https://github.com/danpovey/lilcom/pull/20#discussion_r339251478:

@@ -0,0 +1,350 @@ +#!/usr/bin/python +""" +Details about the Doument: ///// COMPLETE

is this or reconstructin-test supposed to be here? Seems to be a duplication.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/danpovey/lilcom/pull/20?email_source=notifications&email_token=AB7HSSHO2TCJODS6RDTJE73QQNS27A5CNFSM4JFE6MKKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJJZSIQ#pullrequestreview-307468578, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB7HSSBMP2QDUBH4QAXX7WLQQNS27ANCNFSM4JFE6MKA .

soroushzargar commented 5 years ago

But I don't know why in thw merge it seemed to be redundant, maybe there is a problem with removal. It may not have been removed in commits.

On Sat, 26 Oct 2019, 08:16 Soroush Zargar, soroushzargar@gmail.com wrote:

Yes in the branch I totally removed the previous file and wrote the code again.

On Sat, 26 Oct 2019, 01:20 Daniel Povey, notifications@github.com wrote:

@danpovey commented on this pull request.

In test/test_reconstruction.py https://github.com/danpovey/lilcom/pull/20#discussion_r339251478:

@@ -0,0 +1,350 @@ +#!/usr/bin/python +""" +Details about the Doument: ///// COMPLETE

is this or reconstructin-test supposed to be here? Seems to be a duplication.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/danpovey/lilcom/pull/20?email_source=notifications&email_token=AB7HSSHO2TCJODS6RDTJE73QQNS27A5CNFSM4JFE6MKKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJJZSIQ#pullrequestreview-307468578, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB7HSSBMP2QDUBH4QAXX7WLQQNS27ANCNFSM4JFE6MKA .

danpovey commented 5 years ago

I wanted to try removing whichever one was not supposed to be there but I can't find out which is the real one because there is no run.sh that would indicate the correct usage to test it. It's just git rm to remove a file.

soroushzargar commented 5 years ago

I don't know what was the cause of this but I handled it. Now it is clear. The file is runnable. Actually, I did not rename it to run.sh in order to be in the same shape as other tests.

danpovey commented 5 years ago

If it is a python script it should have the suffix .py.

Perhaps I was not clear. I want there to be a top-level shell script- say, run.sh, or run_comparison.sh - that downloads the data from openslr if it is not already downloaded, and runs that python script with appropriate args. Right now it's not at all easy for someone unfamiliar with the project to do that. So the user would just do

cd tests
 ./run_comparison.sh

to run that test. Also please finish the doc-string at the top, right now it says COMPLETE. Let me know if you think you understand what I am asking you to do.

soroushzargar commented 5 years ago

Yes, Actually. So by now, just the script should download the dataset (if not downloaded) and place it near the code, then it should run the test (as it does). At last the docstring should be completed. Ok, I'm on it. Just there might be a problem for me today, In case I will try to deliver it tomorrow.

Soroush Haj Zargarbashi M.Sc in Computer Science University of Tehran

On Sun, Oct 27, 2019 at 10:40 PM Daniel Povey notifications@github.com wrote:

If it is a python script it should have the suffix .py.

Perhaps I was not clear. I want there to be a top-level shell script- say, run.sh, or run_comparison.sh - that downloads the data from openslr if it is not already downloaded, and runs that python script with appropriate args. Right now it's not at all easy for someone unfamiliar with the project to do that. So the user would just do

cd tests ./run_comparison.sh

to run that test. Also please finish the doc-string at the top, right now it says COMPLETE. Let me know if you think you understand what I am asking you to do.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/danpovey/lilcom/pull/20?email_source=notifications&email_token=AB7HSSDB4FLAAUDO7SEXN2LQQXRR3A5CNFSM4JFE6MKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECLFSLI#issuecomment-546724141, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB7HSSAVM4QJLKS4JPM4UKLQQXRR3ANCNFSM4JFE6MKA .

soroushzargar commented 5 years ago

some errors in different sample rates occurred. BTW the shell file is being written. It also installs all the needed packages in case of absence.

soroushzargar commented 5 years ago

I'll go for checking the problem with sample rate I'll create another PR if you considered merging this.

danpovey commented 4 years ago

I pushed a change RE readability. I think you may be just trusting that mp3 is using the bitrates that you direct? It may not, at all sample rates; you should measure the num-bytes in the file.

On Tue, Oct 29, 2019 at 5:33 AM Soroush Zargar notifications@github.com wrote:

I'll go for checking the problem with sample rate I'll create another PR if you considered merging this.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/danpovey/lilcom/pull/20?email_source=notifications&email_token=AAZFLO4YV7WJCXVROE75I7DQRAUSFA5CNFSM4JFE6MKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECQKAGY#issuecomment-547397659, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZFLO46K25ON24UTBOY6YTQRAUSFANCNFSM4JFE6MKA .

soroushzargar commented 4 years ago

Sorry, but I thought that the bitrate name as standard. If you wouldn't mind I'll fix this in a day or two. Sorry but now it's a super hard time for me. I'll get to it soon.

danpovey commented 4 years ago

Thanks.

On Sun, Nov 3, 2019 at 12:10 AM Soroush Zargar notifications@github.com wrote:

Sorry, but I thought that the bitrate name as standard. If you wouldn't mind I'll fix this in a day or two. Sorry but now it's a super hard time for me. I'll get to it soon.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/danpovey/lilcom/pull/20?email_source=notifications&email_token=AAZFLOY7ROQ6NPNNPOM3RHLQRZ2PDA5CNFSM4JFE6MKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC5MLWQ#issuecomment-549111258, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZFLO573LHNDZKQECRPG5LQRZ2PDANCNFSM4JFE6MKA .