Skip to content
Snippets Groups Projects

Add audio extractor

Add audio extractor
add_audio_extractor into master
1 open thread
Merged
Saeed SARFJOOrequested to merge
add_audio_extractor into master
1 open thread
  • Add audio extractor to bob.learn.pytorch
  • Update the unit tests
  • Update doc
  • Resolve #9 (closed)
Edited by Saeed SARFJOO

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Guillaume HEUSCH
    Guillaume HEUSCH @heusch
    Maintainer

    Hi @ssarfjoo,

    Thanks for opening the MR. A better approach would be to open it as soon as you start working on that (instead on when you think it is ready to merge), so that it is easier to follow and comment as code evolves.

    I already have a few comments, but I may not have the time to go through all the code right now, and properly address that this morning, so stay tuned ;)

    Thanks

  • Saeed SARFJOO
    Saeed SARFJOO @ssarfjoo
    Author Developer

    Thanks for reviewing this. I'll wait for your comments.

  • Guillaume HEUSCH
    Guillaume HEUSCH @heusch
    Maintainer

    Alright, so I had time to have a look ... Here are a few comments and suggestions.

    1. Docstrings (in numpy format) should be put wherever not present in order to generate useful python API documentation (i.e. what a class is, what a function is doing and what parameters it expects, and so on). You actually did it well for preprocessor/audio/DltResNet

    2. There is a lot of redundanxcy between your ResNet and DltResNet classes (I actually don't see much difference) - could you make it one class only by setting a flag if you want to build the DltResNet or the ResNet.

    Also, you should move common (and more generic) classes BasicBlock and Bottleneck) to architectures/utils and import from there. This way, one can easily reuse them if needed in another architecture

    1. spectrogram_utils has basically nothing to do with neural networks, it should be located elsewhere (bob.pad.voice or bob.bio.spear, or even bob.ap). This would help this package remain more specific and avoid additional requirements like scipy.io.wavfile.

    2. Use the Logging module instead of print statements

    That's basically what can be improved I guess.

    Additionally, I find your code clean and well organized, thanks ! (I'm by no means a software design - nor developer expert though ;)

  • Saeed SARFJOO added 27 commits

    added 27 commits

    Compare with previous version

    Toggle commit list
  • Guillaume HEUSCH
    Guillaume HEUSCH @heusch started a thread on an old version of the diff
bob/learn/pytorch/architectures/DltResNet.py 0 → 100644
27 logger = logging.getLogger("bob.learn.pytorch")
28
29 __all__ = ['ResNet', 'resnet18', 'resnet34', 'resnet50', 'resnet101',
30 'resnet152']
31
32
33 model_urls = {
34 'resnet18': 'https://s3.amazonaws.com/pytorch/models/resnet18-5c106cde.pth',
35 'resnet34': 'https://s3.amazonaws.com/pytorch/models/resnet34-333f7ec4.pth',
36 'resnet50': 'https://s3.amazonaws.com/pytorch/models/resnet50-19c8e357.pth',
37 'resnet101': 'https://s3.amazonaws.com/pytorch/models/resnet101-5d3b4d8f.pth',
38 'resnet152': 'https://s3.amazonaws.com/pytorch/models/resnet152-b121ed2d.pth',
39 }
40
41
42 class ResNet(nn.Module):
  • Guillaume HEUSCH marked as a Work In Progress

    marked as a Work In Progress

  • Guillaume HEUSCH changed the description

    changed the description

  • Saeed SARFJOO
    Saeed SARFJOO @ssarfjoo
    Author Developer

    Okay I will change the class name to DltResNet in DltResNet.py file. However, because dilation can be added in each layer, this is better to keep these two classes separately. But in preprocessor I removed the ResNet.py file.

  • Saeed SARFJOO added 1 commit

    added 1 commit

    Compare with previous version

  • Saeed SARFJOO added 1 commit

    added 1 commit

    • b6392865 - use bob.ap for spectrogram extraction

    Compare with previous version

  • Saeed SARFJOO added 31 commits

    added 31 commits

    Compare with previous version

    Toggle commit list
  • Saeed SARFJOO added 1 commit

    added 1 commit

    Compare with previous version

  • Saeed SARFJOO added 1 commit

    added 1 commit

    • 6c2dac5f - remove extend_path from preprocessor

    Compare with previous version

  • Saeed SARFJOO added 1 commit

    added 1 commit

    • e3bebdc4 - remove extend_path from all internal __init__ files

    Compare with previous version

  • Saeed SARFJOO
    Saeed SARFJOO @ssarfjoo
    Author Developer

    @heusch I applied the requested modifications. In addition I removed extend_path from all internal __init__ files.

  • Saeed SARFJOO unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Saeed SARFJOO changed the description

    changed the description

  • Guillaume HEUSCH
  • Guillaume HEUSCH
  • Guillaume HEUSCH
  • Guillaume HEUSCH
    Guillaume HEUSCH @heusch
    Maintainer

    Other than what is discussed above, this looks ok to me ! Thanks

  • Saeed SARFJOO added 1 commit

    added 1 commit

    • 7e4711be - move _make_layer function and update doc

    Compare with previous version

  • Saeed SARFJOO added 1 commit

    added 1 commit

    • 1710ffde - fix input problem in new _make_layer function

    Compare with previous version

  • Saeed SARFJOO
    Saeed SARFJOO @ssarfjoo
    Author Developer

    @heusch I applied the requested modifications. Please merge it if you think that's okay.

  • Guillaume HEUSCH
    Guillaume HEUSCH @heusch
    Maintainer

    All right, thanks for the modifications, I'm merging !

  • Guillaume HEUSCH closed

    closed

  • Guillaume HEUSCH reopened

    reopened

  • Guillaume HEUSCH merged

    merged

  • Guillaume HEUSCH mentioned in commit a7c84482

    mentioned in commit a7c84482

    • Please register or sign in to reply