Added TCN Carnatic beat tracking module#113
Conversation
thomasgnuttall
left a comment
thomasgnuttall
left a comment
There was a problem hiding this comment.
Thanks Vivek. I have tested the model and it looks good! I have requested a small change regarding hardcoded parameters. These should be abstracted out so as to be passed to the top level API. i.e. TCNTracker()
| import numpy as np | ||
|
|
||
| fps= 100 # frames per second for the DBN processors | ||
| min_bpm= 55.0 |
There was a problem hiding this comment.
These should be parameters of the model, not hardcoded
| def beat_tracker(beats_act): | ||
| beats_act = clip_probabilities(beats_act) | ||
| beat_dbn = DBNBeatTrackingProcessor( | ||
| min_bpm=min_bpm, max_bpm=max_bpm, fps=fps, transition_lambda=100, online=False) |
There was a problem hiding this comment.
Other model parameters, not hardcoded.
|
|
||
| class PreProcessor(SequentialProcessor): | ||
| def __init__(self, frame_size=2048, num_bands=12, log=np.log, add=1e-6, fps=100): | ||
| sig = SignalProcessor(num_channels=1, sample_rate=44100) |
There was a problem hiding this comment.
Is this hardcoded going to cause a problem for datasets not at 44100Hz?
tests/rhythm/test_tcn_carnatic.py
Outdated
| from compiam.data import TESTDIR | ||
| from compiam.exceptions import ModelNotTrainedError | ||
|
|
||
|
|
| "essentia", | ||
| "full_ml", | ||
| "all", | ||
| "madmom @ git+https://github.com/vivekvjyn/madmom.git" |
There was a problem hiding this comment.
We can allow this for now but perhaps should think about moving this to the MTG workspace
|
They are not parameters of the model, they are parameters for the madmom post-processing. A carnatic performance could feasibly be at a tempo higher than 230bpm and whilst the the results might be optimal for the range studied in the paper it would be good to give users the flexbility to decide for themselves if the model is valuable at higher bpms. I would pass them as parameters to the process and set the defaults to those in the paper, which is generally a good development practice anyway. I think fps is not so important but min_bpm, max_bpm, and beats_per_bar should be optional parameters for the higher-level API. Thanks for the sr change! |
|
Please go through the code and ensure that sample rate is not hardcoded (for example in pre.py) |
thomasgnuttall
left a comment
thomasgnuttall
left a comment
There was a problem hiding this comment.
See previous comments
|
there are no hard-coded parameters now, except for |
2 participants
compiam.rhythm.meter.tcn_carnatic.TCNTrackerTCNTracker.__int__()by passingmodel_version= 42, 52, 62. which are the seeds each model was trained on (as per the paper).madmomfork in my github temporarilynumpydowngrade tonumpy==1.23.5, which works for all other modules.