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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subspace noise removal #73

Merged
merged 5 commits into from Jan 8, 2019
Merged

Conversation

ebezzam
Copy link
Member

@ebezzam ebezzam commented Jan 7, 2019

This PR includes work from this mini-project by @GimenaSegrelles and Mathieu Lecoq for the EPFL audio course.


Thanks for sending a pull request (PR), we really appreciate that! Before hitting the submit button, we'd be really glad if you could make sure all the following points have been cleared.

Please also refer to the doc on contributing for more details. Even if you can't check all the boxes below, do not hesitate to go ahead with the PR and ask for help there.

  • Are there docstrings ? Do they follow the numpydoc style ?
  • Have you run the tests by doing nosetests or py.test at the root of the repo ?
  • Have you checked that the doc builds properly and that any new file has been added to the repo ? How to do that is covered in the documentation.
  • Is there a unit test for the proposed code modification ? If the PR addresses an issue, the test should make sure the issue is fixed.
  • Last but not least, did you document the proposed change in the CHANGELOG file ? It should go under "Unreleased".

Happy PR 馃槂

I ran into an error when running nosetests on the DFT unit tests... @fakufaku have you had this?

malloc: *** error for object 0x7fddad8f9748: incorrect checksum for freed object - object was probably modified after being freed.

@ebezzam ebezzam requested a review from fakufaku January 7, 2019 09:30
@fakufaku
Copy link
Collaborator

fakufaku commented Jan 8, 2019

Pretty cool! I didn't know about the subspace denoising method. I will look at it!

I have been having some issues with nosetests depending on the conda environment used. Sometimes the environment gets corrupted and I get weird errors. I would try to create a fresh environment with only the minimal dependencies of the pyroomacoustics test suite and try running the tests again.

One thing that happen sometimes is that I use a conda env that doesn't have nosetests installed. But if nosetests is installed in the root environment, then the root nosetests is run within the test env, which creates problems. If nosetests is not installed in the test env, installing it might make the problem go away.

Before merging this commit, could you please make sure all the code is properly attributed to the authors (@GimenaSegrelles and Mathieu Lecoq, and yourself where necessary) ? I think we need to add the attribution in comments at the top of the actual file. In addition, I think it would be nice to keep a file with the names of the contributors at the root of the repo. At the moment, it is a bit disparate. We have some names in the files, some names in the README (which should just be the core developers), and some commit information, which is only in git. In this PR, just do this for the added code. We can revisit the rest later.

@ebezzam
Copy link
Member Author

ebezzam commented Jan 8, 2019

Yeah it's an interesting approach working in the time domain!

For the author attributes, you mean like at the top of this file?

Thanks for the nosetests tip, I'll try this out 馃檪

@fakufaku
Copy link
Collaborator

fakufaku commented Jan 8, 2019

Yes, there should at least be something like this. Having the disclaimer from the license might even be a good idea.

@ebezzam
Copy link
Member Author

ebezzam commented Jan 8, 2019

The text from this file?

@ebezzam
Copy link
Member Author

ebezzam commented Jan 8, 2019

I did something like the top answer on this post, using MIT license instead.

@fakufaku
Copy link
Collaborator

fakufaku commented Jan 8, 2019

Thanks! But... 馃檲 I am just thinking that if we do it this way the boring license stuff will show up on every page of the doc.

It seems a solution is to put the license info before the docstring in lines commented with # (https://stackoverflow.com/questions/17209712/how-do-i-get-sphinx-to-ignore-the-gpl-notice-in-my-docstring)

# Boring
# License
# stuff (c)
'''
The docstring starts here

...
'''

@ebezzam
Copy link
Member Author

ebezzam commented Jan 8, 2019

Oops didn't think about it popping up in the docs!

@fakufaku
Copy link
Collaborator

fakufaku commented Jan 8, 2019

Thanks, did you check this doesn't show up in a local build of the doc ?

@ebezzam
Copy link
Member Author

ebezzam commented Jan 8, 2019

Yes, it doesn't show up with #. I also tried building the doc with the """ ...""" comment style and the "boring license stuff" indeed showed up.

Copy link
Collaborator

@fakufaku fakufaku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great! The merge can go ahead!

@fakufaku
Copy link
Collaborator

fakufaku commented Jan 8, 2019

Oups, found a typo in the references of the notebook of the mini-project, not sure whether it made it into this PR or not:

A Signal Subspace Approach for Speech Enhancement Yariv Ephraim, Fellow, IEEE, and Hany L. Van Trees, Life Fellow, IEEE, TRANSACTIONS ON SPEECH AND AUDIO PROCESSING, VOL. 3, NO. 4, JULY 1995

This should be Harry L. Van Trees.

@fakufaku
Copy link
Collaborator

fakufaku commented Jan 8, 2019

It seems the only mention is as H. L. Van Trees in the current code, so all is good. I'll merge this.

@fakufaku fakufaku merged commit 692d113 into LCAV:master Jan 8, 2019
@fakufaku
Copy link
Collaborator

fakufaku commented Jan 8, 2019

Done, thanks @ebezzam !

@ebezzam
Copy link
Member Author

ebezzam commented Jan 8, 2019

Thanks @fakufaku, and to @GimenaSegrelles and Mathieu Lecoq for their work during the semester!

ebezzam added a commit to ebezzam/pyroomacoustics that referenced this pull request Feb 6, 2019
* Add subspace denoising approach.
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

Successfully merging this pull request may close these issues.

None yet

2 participants