-
Notifications
You must be signed in to change notification settings - Fork 59
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
gtzan_genre.Track.to_jams #212
Conversation
Codecov Report
@@ Coverage Diff @@
## master #212 +/- ##
==========================================
+ Coverage 95.90% 95.98% +0.07%
==========================================
Files 19 19
Lines 1905 1942 +37
==========================================
+ Hits 1827 1864 +37
Misses 78 78 |
mirdata/gtzan_genre.py
Outdated
raise NotImplementedError | ||
"""Jams: the track's data in jams format""" | ||
# Initialize top-level JAMS container | ||
jam = jams.JAMS() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use the mirdata.jams_utils.jams_converter
function. If it's missing fields, we should add them to the converter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rabitt so i should make a gtzan_to_jams
function in jams_utils
? and pass a gtzan_data
(default None
) to the jams_converter
? that sounds overkill to me, but i can live with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good, except that the to_jams method should use the built in jams converter function. Otherwise it looks great.
A caveat - the big change I pushed results in a less complete jams object in the |
'tags_data should be a list of tuples, ' | ||
+ 'but contains a {} element'.format(type(tag)) | ||
) | ||
jam.annotations.append(tag_gtzan_to_jams(tag)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tag_gtzan_to_jams
returns an Annotation
in the gtzan
namespace. This Annotation
will be JAMS-compliant if and only if the tag
is a GTZAN genre: country, jazz, rock, etc.
This tag converter will not work for other datasets than GTZAN (e.g. MSD, which requires tag_open
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - this is why I called the method "tag_gtzan_to_jams"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see though - the value is called "tags_data" so it's confusing. I'll update.
gtzan_genre.Track.to_jams (#212) * fix GTZAN capitalization in docstring * Implement gtzan_genre.Track.to_jams * black -S . gtzan_genre * test JAMS parser for GTZAN * import jams in gtzan_genre * import mirdata in gtzan_genre * define VERSION and ANNOTATION_RULES for gtzan_genre * fixes #217; adds test that jams validation succeeds for all datasets; adds duration to most to_jams() methods; uses jams_converter in gtzan_genre * run black * update tags_data to tags_gtzan_data authored-by: Rachel Bittner <rmb456@nyu.edu>
This PR implements a JAMS parser for
gtzan_genre
, in the prospect of closing #197 ("DALI and GTZAN Genre lack a to_jams method")This was quite easy. I am going to write unit tests now.
PS: nevermind that this branch is called
dali-jams
. This is agtzan_genre
PR.EDIT: [rabitt]
This PR now does the following:
test_loaders
which ensures that the output of everyto_jams()
method successfully validatesduration
was not part of the file_metadata. I've added duration to everyto_jams
method.jams_utils.jams_converter
for gtzan tags.jams_utils
to always usif not isinstance(...)
overif type(...) != ...