pyroomacoustics icon indicating copy to clipboard operation
pyroomacoustics copied to clipboard

DIRPAT RIR Code First Commit Review

Open prerak23 opened this issue 2 years ago • 4 comments

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 :smiley:

prerak23 avatar May 17 '22 11:05 prerak23

Hi @prerak23 ! Thank you so much for the PR! I will review it in the coming days! 👍

fakufaku avatar May 21 '22 04:05 fakufaku

@prerak23 I think the import of file open_sofa_interpolate does not have the correct syntax. See the test output here

pyroomacoustics/directivities.py:9: in <module>
    import open_sofa_interpolate
E   ModuleNotFoundError: No module named 'open_sofa_interpolate'

It should be done as follows

from . import open_sofa_interpolate

fakufaku avatar May 22 '22 13:05 fakufaku

Thanks, i will note all the given corrections as the review of the pull request goes ahead . But yes the DIRPAT sofa files are not present in the github repo. You can download them from here. https://phaidra.kug.ac.at/detail/o:68229#?q=DIRPAT&page=1&pagesize=10

prerak23 avatar May 23 '22 15:05 prerak23

@prerak23 I think we are close but

  1. You have included 1600+ files (from libroom/ext/eigen) in the commit. Could you please remove them ? Also, before committing, I would suggest that you review what files you are about to commit with the git status command.
  2. You need to add import os in the import section of your test files.

fakufaku avatar Aug 20 '22 03:08 fakufaku