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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MultiAnnotator class #515

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

magdalenafuentes
Copy link
Collaborator

@magdalenafuentes magdalenafuentes commented Jun 3, 2021

This PR adds the MultiAnnotator class to group annotations of same track by different annotators in a smooth and clear way.

IMPORTANT: this PR deprecates the API for the salami loader, but is still compatible to it.

@codecov
Copy link

codecov bot commented Jun 3, 2021

Codecov Report

Merging #515 (f37f5ba) into master (3fab998) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #515   +/-   ##
=======================================
  Coverage   99.02%   99.02%           
=======================================
  Files          44       44           
  Lines        5224     5244   +20     
=======================================
+ Hits         5173     5193   +20     
  Misses         51       51           

Copy link
Collaborator

@nkundiushuti nkundiushuti left a comment

Choose a reason for hiding this comment

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

maybe add something to the contributing documentation?

mirdata/annotations.py Outdated Show resolved Hide resolved
@@ -1392,11 +1414,12 @@ def validate_array_like(array_like, expected_type, expected_dtype, none_allowed=
array_like (array-like): object to validate
expected_type (type): expected type, either list or np.ndarray
expected_dtype (type): expected dtype
check_child (bool): if True, checks if all elements of array are children of expected_dtype
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if there are multiple children with different expected dtypes (e.g a beat annotation with ints for the beat positions and floats for the time stamps?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

here we're only checking if the list is composed of BeatData, ChordData, etc. The checks within the annotation type are done for each annotation type independently

mirdata/datasets/salami.py Show resolved Hide resolved
mirdata/annotations.py Outdated Show resolved Hide resolved
mirdata/annotations.py Show resolved Hide resolved
annotations (list): list of annotations (e.g. [annotations.BeatData, annotations.ChordData]
"""

def __init__(self, annotators, annotations) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these two inputs would be better as a single dictionary:
{ 'annotator-id': Annotation }

same with how it's stored, self.annotations = {'annotator1': ... , 'annotator2', ...}
because it will make it easier to look up a specific dictionary instead of having to look for matching indexes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried this out and noticed that some tracks have the same annotator id for different annotations in the same track (which might be a bit weird actually... but it's there :/). Also the sintax gets a bit messy, since I have to iterate in the keys each time I want to access an annotation because the annotator id changes from track to track. So I changed back to what was before for now. Let me know what you think

mirdata/annotations.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@nkundiushuti nkundiushuti left a comment

Choose a reason for hiding this comment

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

this looks great! thanks, @magdalenafuentes !

@magdalenafuentes
Copy link
Collaborator Author

Will wait for @rabitt to see if she has any other comments/edits. Otherwise @nkundiushuti or @rabitt feel free to merge since I will be offline starting today!

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.

3 participants