Repository: mir-dataset-loaders/mirdata

Issue:
Jams object completeness
For ease of contribution, we've implemented a `jams_utils.jams_converter` function which writes jams objects under the hood for each Track object's `.to_jams()` method. This has two benefits:
* contributors do not need to be familiar with the jams api
* each dataset module's code is drastically simplified.

However, this limits the completeness of the resulting jams object - namely, we omit much of the metadata that could be included. We now enforce that the jams objects returned by `Track.to_jams()` passes validation, so they are minimally usable, but they are not ideal.

Do we have any ideas for how to improve the jams object completeness without hurting the ease of contribution? Do we need fully complete jams files for most use cases (e.g. usage with `muda`)

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/mir-dataset-loaders/mirdata/pull/247?src=pr&el=h1) Report
> Merging [#247](https://codecov.io/gh/mir-dataset-loaders/mirdata/pull/247?src=pr&el=desc) into [master](https://codecov.io/gh/mir-dataset-loaders/mirdata/commit/ec3eb7820885cad4f1586a6bf493b85c431088e9&el=desc) will **decrease** coverage by `0.05%`.
> The diff coverage is `93.33%`.


```diff
@@            Coverage Diff             @@
##           master     #247      +/-   ##
==========================================
- Coverage   98.78%   98.73%   -0.06%     
==========================================
  Files          20       20              
  Lines        2136     2128       -8     
==========================================
- Hits         2110     2101       -9     
- Misses         26       27       +1     
```


Patch:
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index c59ed54..05d2dd6 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -208,9 +208,9 @@ class Track(track.Track):
             self._data_home, self._track_paths['annotation'][0])
 
         # -- if the user doesn't have a metadata file, load None
-        metadata = DATA.metadata(data_home)
-        if metadata is not None and track_id in metadata:
-            self.some_metadata = metadata[track_id]['some_metadata']
+        self._metadata = DATA.metadata(data_home)
+        if self._metadata is not None and track_id in self._metadata:
+            self.some_metadata = self._metadata[track_id]['some_metadata']
         else:
             self.some_metadata = None
 
@@ -237,10 +237,12 @@ class Track(track.Track):
     # -- object with the annotations.
     def to_jams(self):
         """Jams: the track's data in jams format"""
-        return jams_utils.jams_convertrer(
+        return jams_utils.jams_converter(
+            audio_path=self.audio_path,
             annotation_data=[(self.annotation, None)],
-            metadata=metadata},
+            metadata=self._metadata,
         )
+        # -- see the documentation for `jams_utils.jams_converter for all fields
 
 
 def load_audio(audio_path):
diff --git a/mirdata/beatles.py b/mirdata/beatles.py
index c42dbb1..ea88722 100644
--- a/mirdata/beatles.py
+++ b/mirdata/beatles.py
@@ -102,15 +102,12 @@ class Track(track.Track):
     def to_jams(self):
         """Jams: the track's data in jams format"""
         return jams_utils.jams_converter(
+            audio_path=self.audio_path,
             beat_data=[(self.beats, None)],
             section_data=[(self.sections, None)],
             chord_data=[(self.chords, None)],
             key_data=[(self.key, None)],
-            metadata={
-                'artist': 'The Beatles',
-                'title': self.title,
-                'duration': librosa.get_duration(self.audio[0], self.audio[1]),
-            },
+            metadata={'artist': 'The Beatles', 'title': self.title},
         )
 
 
diff --git a/mirdata/dali.py b/mirdata/dali.py
index 9b4dc3e..1dda3fc 100644
--- a/mirdata/dali.py
+++ b/mirdata/dali.py
@@ -173,18 +173,15 @@ class Track(track.Track):
 
     def to_jams(self):
         """Jams: the track's data in jams format"""
-        # Load metadata
-        metadata = {k: v for k, v in self._track_metadata.items() if v is not None}
-        y, sr = self.audio
-        metadata['duration'] = librosa.get_duration(y=y, sr=sr)
         return jams_utils.jams_converter(
+            audio_path=self.audio_path,
             lyrics_data=[
                 (self.words, 'word-aligned lyrics'),
                 (self.lines, 'line-aligned lyrics'),
                 (self.paragraphs, 'paragraph-aligned lyrics'),
             ],
             note_data=[(self.notes, 'annotated vocal notes')],
-            metadata=metadata,
+            metadata=self._track_metadata,
         )
 
 
diff --git a/mirdata/ikala.py b/mirdata/ikala.py
index bb21bdd..30bde48 100644
--- a/mirdata/ikala.py
+++ b/mirdata/ikala.py
@@ -135,6 +135,7 @@ class Track(track.Track):
     def to_jams(self):
         """Jams: the track's data in jams format"""
         return jams_utils.jams_converter(
+            audio_path=self.audio_path,
             f0_data=[(self.f0, None)],
             lyrics_data=[(self.lyrics, None)],
             metadata={
@@ -142,7 +143,6 @@ class Track(track.Track):
                 'singer_id': self.singer_id,
                 'track_id': self.track_id,
                 'song_id': self.song_id,
-                'duration': librosa.get_duration(self.mix_audio[0], self.mix_audio[1]),
             },
         )
 
diff --git a/mirdata/jams_utils.py b/mirdata/jams_utils.py
index ea5acbd..5caf330 100644
--- a/mirdata/jams_utils.py
+++ b/mirdata/jams_utils.py
@@ -3,10 +3,14 @@
 """
 
 import jams
+import librosa
+import os
+
 from mirdata import utils
 
 
 def jams_converter(
+    audio_path=None,
     beat_data=None,
     chord_data=None,
     note_data=None,
@@ -24,6 +28,11 @@ def jams_converter(
 
     Parameters
     ----------
+    audio_path (str or None):
+        A path to the corresponding audio file, or None. If provided,
+        the audio file will be read to compute the duration. If None,
+        'duration' must be a field in the metadata dictionary, or the
+        resulting jam object will not validate.
     beat_data (list or None):
         A list of tuples of (BeatData, str), where str describes the annotation (e.g. 'beats_1').
     chord_data (list or None):
@@ -62,14 +71,39 @@ def jams_converter(
 
     jam = jams.JAMS()
 
+    # duration
+    duration = None
+    if audio_path is not None:
+        if os.path.exists(audio_path):
+            duration = librosa.get_duration(filename=audio_path)
+        else:
+            raise OSError(
+                'jams conversion failed because the audio file '
+                + 'for this track cannot be found, and it is required'
+                + 'to compute duration.'
+            )
+
     # metadata
     if metadata is not None:
         for key in metadata:
+            if key == 'duration' and duration is not None and metadata[key] != duration:
+                print(
+                    'Warning: duration provided in metadata does not'
+                    + 'match the duration computed from the audio file.'
+                    + 'Using the duration provided by the metadata.'
+                )
+
+            if metadata[key] is None:
+                continue
+
             if hasattr(jam.file_metadata, key):
                 setattr(jam.file_metadata, key, metadata[key])
             else:
                 setattr(jam.sandbox, key, metadata[key])
 
+    if jam.file_metadata.duration is None:
+        jam.file_metadata.duration = duration
+
     # beats
     if beat_data is not None:
         if not isinstance(beat_data, list):
diff --git a/mirdata/medley_solos_db.py b/mirdata/medley_solos_db.py
index 8f9d67f..5732286 100644
--- a/mirdata/medley_solos_db.py
+++ b/mirdata/medley_solos_db.py
@@ -137,9 +137,9 @@ class Track(track.Track):
 
     def to_jams(self):
         """Jams: the track's data in jams format"""
-        metadata = {'duration': librosa.get_duration(self.audio[0], self.audio[1])}
-        metadata.update(self._track_metadata)
-        return jams_utils.jams_converter(metadata=metadata)
+        return jams_utils.jams_converter(
+            audio_path=self.audio_path, metadata=self._track_metadata
+        )
 
 
 def load_audio(audio_path):
diff --git a/mirdata/medleydb_melody.py b/mirdata/medleydb_melody.py
index 8f4304c..0f10928 100644
--- a/mirdata/medleydb_melody.py
+++ b/mirdata/medleydb_melody.py
@@ -133,11 +133,10 @@ class Track(track.Track):
     def to_jams(self):
         """Jams: the track's data in jams format"""
         # jams does not support multipitch, so we skip melody3
-        metadata = {k: v for k, v in self._track_metadata.items() if v is not None}
-        metadata['duration'] = librosa.get_duration(self.audio[0], self.audio[1])
         return jams_utils.jams_converter(
+            audio_path=self.audio_path,
             f0_data=[(self.melody1, 'melody1'), (self.melody2, 'melody2')],
-            metadata=metadata,
+            metadata=self._track_metadata,
         )
 
 
diff --git a/mirdata/medleydb_pitch.py b/mirdata/medleydb_pitch.py
index 147aa31..2b58396 100644
--- a/mirdata/medleydb_pitch.py
+++ b/mirdata/medleydb_pitch.py
@@ -106,10 +106,10 @@ class Track(track.Track):
 
     def to_jams(self):
         """Jams: the track's data in jams format"""
-        metadata = {k: v for k, v in self._track_metadata.items() if v is not None}
-        metadata['duration'] = librosa.get_duration(self.audio[0], self.audio[1])
         return jams_utils.jams_converter(
-            f0_data=[(self.pitch, 'annotated pitch')], metadata=metadata
+            audio_path=self.audio_path,
+            f0_data=[(self.pitch, 'annotated pitch')],
+            metadata=self._track_metadata,
         )
 
 
diff --git a/mirdata/orchset.py b/mirdata/orchset.py
index bf870af..4d16caa 100644
--- a/mirdata/orchset.py
+++ b/mirdata/orchset.py
@@ -195,12 +195,10 @@ class Track(track.Track):
 
     def to_jams(self):
         """Jams: the track's data in jams format"""
-        metadata = {k: v for k, v in self._track_metadata.items() if v is not None}
-        metadata['duration'] = librosa.get_duration(
-            self.audio_mono[0], self.audio_mono[1]
-        )
         return jams_utils.jams_converter(
-            f0_data=[(self.melody, 'annotated melody')], metadata=metadata
+            audio_path=self.audio_path_mono,
+            f0_data=[(self.melody, 'annotated melody')],
+            metadata=self._track_metadata,
         )
 
 
diff --git a/mirdata/rwc_classical.py b/mirdata/rwc_classical.py
index 608a8b5..6cb6f17 100644
--- a/mirdata/rwc_classical.py
+++ b/mirdata/rwc_classical.py
@@ -178,12 +178,11 @@ class Track(track.Track):
 
     def to_jams(self):
         """Jams: the track's data in jams format"""
-        metadata = {k: v for k, v in self._track_metadata.items() if v is not None}
-        metadata['duration'] = librosa.get_duration(self.audio[0], self.audio[1])
         return jams_utils.jams_converter(
+            audio_path=self.audio_path,
             beat_data=[(self.beats, None)],
             section_data=[(self.sections, None)],
-            metadata=metadata,
+            metadata=self._track_metadata,
         )
 
 
diff --git a/mirdata/rwc_jazz.py b/mirdata/rwc_jazz.py
index f59c862..56fe7ed 100644
--- a/mirdata/rwc_jazz.py
+++ b/mirdata/rwc_jazz.py
@@ -203,12 +203,11 @@ class Track(track.Track):
 
     def to_jams(self):
         """Jams: the track's data in jams format"""
-        metadata = {k: v for k, v in self._track_metadata.items() if v is not None}
-        metadata['duration'] = librosa.get_duration(self.audio[0], self.audio[1])
         return jams_utils.jams_converter(
+            audio_path=self.audio_path,
             beat_data=[(self.beats, None)],
             section_data=[(self.sections, None)],
-            metadata=metadata,
+            metadata=self._track_metadata,
         )
 
 
diff --git a/mirdata/rwc_popular.py b/mirdata/rwc_popular.py
index 3ce1291..8122219 100644
--- a/mirdata/rwc_popular.py
+++ b/mirdata/rwc_popular.py
@@ -221,13 +221,12 @@ class Track(track.Track):
 
     def to_jams(self):
         """Jams: the track's data in jams format"""
-        metadata = {k: v for k, v in self._track_metadata.items() if v is not None}
-        metadata['duration'] = librosa.get_duration(self.audio[0], self.audio[1])
         return jams_utils.jams_converter(
+            audio_path=self.audio_path,
             beat_data=[(self.beats, None)],
             section_data=[(self.sections, None)],
             chord_data=[(self.chords, None)],
-            metadata=metadata,
+            metadata=self._track_metadata,
         )
 
 
diff --git a/mirdata/salami.py b/mirdata/salami.py
index 00bdfb9..4aeb6c4 100644
--- a/mirdata/salami.py
+++ b/mirdata/salami.py
@@ -197,9 +197,8 @@ class Track(track.Track):
 
     def to_jams(self):
         """Jams: the track's data in jams format"""
-        metadata = {k: v for k, v in self._track_metadata.items() if v is not None}
-        metadata['duration'] = librosa.get_duration(self.audio[0], self.audio[1])
         return jams_utils.jams_converter(
+            audio_path=self.audio_path,
             multi_section_data=[
                 (
                     [
@@ -216,7 +215,7 @@ class Track(track.Track):
                     'annotator_2',
                 ),
             ],
-            metadata=metadata,
+            metadata=self._track_metadata,
         )
 
 
diff --git a/mirdata/tinysol.py b/mirdata/tinysol.py
index c0120f8..106e492 100644
--- a/mirdata/tinysol.py
+++ b/mirdata/tinysol.py
@@ -194,9 +194,9 @@ class Track(track.Track):
 
     def to_jams(self):
         """Jams: the track's data in jams format"""
-        metadata = {k: v for k, v in self._track_metadata.items() if v is not None}
-        metadata['duration'] = librosa.get_duration(self.audio[0], self.audio[1])
-        return jams_utils.jams_converter(metadata=metadata)  # TODO PR #185
+        return jams_utils.jams_converter(
+            audio_path=self.audio_path, metadata=self._track_metadata
+        )
 
 
 def load_audio(audio_path):


Test patch:
diff --git a/tests/test_jams_utils.py b/tests/test_jams_utils.py
index 2b0de1e..11dc6d5 100644
--- a/tests/test_jams_utils.py
+++ b/tests/test_jams_utils.py
@@ -977,6 +977,7 @@ def test_tags():
     tag_data3 = [('invalid', 'asdf')]
     tag_data4 = ('jazz', 'wrong format')
     tag_data5 = ['wrong format too']
+    tag_data6 = [(123, 'asdf')]
     jam1 = jams_utils.jams_converter(
         tags_gtzan_data=tag_data1, metadata={'duration': 10.0}
     )
@@ -991,9 +992,11 @@ def test_tags():
     with pytest.raises(jams.SchemaError):
         assert jam3.validate()
     with pytest.raises(TypeError):
-        jam4 = jams_utils.jams_converter(tags_gtzan_data=tag_data4)
+        jams_utils.jams_converter(tags_gtzan_data=tag_data4)
     with pytest.raises(TypeError):
-        jam5 = jams_utils.jams_converter(tags_gtzan_data=tag_data5)
+        jams_utils.jams_converter(tags_gtzan_data=tag_data5)
+    with pytest.raises(TypeError):
+        jams_utils.jams_converter(tags_gtzan_data=tag_data6)
 
 
 def test_tempos():
@@ -1002,6 +1005,7 @@ def test_tempos():
     tempo_data3 = [(-1, 'asdf')]
     tempo_data4 = (120.5, 'wrong format')
     tempo_data5 = ['wrong format too']
+    tempo_data6 = [('string!', 'string!')]
     jam1 = jams_utils.jams_converter(
         tempo_data=tempo_data1, metadata={'duration': 10.0}
     )
@@ -1016,9 +1020,11 @@ def test_tempos():
     with pytest.raises(jams.SchemaError):
         assert jam3.validate()
     with pytest.raises(TypeError):
-        jam4 = jams_utils.jams_converter(tempo_data=tempo_data4)
+        jams_utils.jams_converter(tempo_data=tempo_data4)
+    with pytest.raises(TypeError):
+        jams_utils.jams_converter(tempo_data=tempo_data5)
     with pytest.raises(TypeError):
-        jam5 = jams_utils.jams_converter(tempo_data=tempo_data5)
+        jams_utils.jams_converter(tempo_data=tempo_data6)
 
 
 def test_events():
@@ -1060,6 +1066,7 @@ def test_events():
     ]
     event_data4 = ('jazz', 'wrong format')
     event_data5 = ['wrong format too']
+    event_data6 = [('wrong', 'description')]
     jam1 = jams_utils.jams_converter(
         event_data=event_data1, metadata={'duration': 10.0}
     )
@@ -1074,9 +1081,11 @@ def test_events():
     with pytest.raises(jams.SchemaError):
         assert jam3.validate()
     with pytest.raises(TypeError):
-        jam4 = jams_utils.jams_converter(event_data=event_data4)
+        jams_utils.jams_converter(event_data=event_data4)
+    with pytest.raises(TypeError):
+        jams_utils.jams_converter(event_data=event_data5)
     with pytest.raises(TypeError):
-        jam5 = jams_utils.jams_converter(event_data=event_data5)
+        jams_utils.jams_converter(event_data=event_data6)
 
 
 def test_metadata():
@@ -1093,3 +1102,55 @@ def test_metadata():
     assert jam_1['file_metadata']['artist'] == 'Meatloaf'
     assert jam_1['file_metadata']['duration'] == 1.5
     assert jam_1['sandbox']['favourite_color'] == 'rainbow'
+
+    # test meatadata value None
+    metadata_2 = {
+        'duration': 1.5,
+        'artist': 'breakmaster cylinder',
+        'title': None,
+        'extra': None
+    }
+    jam2 = jams_utils.jams_converter(metadata=metadata_2)
+    assert jam2.validate()
+    assert jam2['file_metadata']['duration'] == 1.5
+    assert jam2['file_metadata']['artist'] == 'breakmaster cylinder'
+    assert jam2['file_metadata']['title'] == ''
+    assert 'extra' not in jam2['sandbox']
+
+
+def test_duration():
+    # duration from audio file
+    jam = jams_utils.jams_converter(
+        audio_path='tests/resources/mir_datasets/iKala/Wavfile/10161_chorus.wav'
+    )
+    assert jam.file_metadata.duration == 2.0
+    assert jam.validate()
+
+    # test invalid file path
+    with pytest.raises(OSError):
+        jams_utils.jams_converter(audio_path='i/dont/exist')
+
+    jam1 = jams_utils.jams_converter(metadata={'duration': 4})
+    assert jam1.file_metadata.duration == 4.0
+    assert jam1.validate()
+
+    # test incomplete metadata
+    jam2 = jams_utils.jams_converter(metadata={'artist': 'b'})
+    with pytest.raises(jams_utils.jams.SchemaError):
+        jam2.validate()
+
+    # test metadata duration and audio file equal
+    jam3 = jams_utils.jams_converter(
+        audio_path='tests/resources/mir_datasets/iKala/Wavfile/10161_chorus.wav',
+        metadata={'duration': 2},
+    )
+    assert jam3.file_metadata.duration == 2
+    assert jam3.validate()
+
+    # test metadata and duration not equal
+    jam4 = jams_utils.jams_converter(
+        audio_path='tests/resources/mir_datasets/iKala/Wavfile/10161_chorus.wav',
+        metadata={'duration': 1000},
+    )
+    assert jam4.file_metadata.duration == 1000
+    assert jam4.validate()
diff --git a/tests/test_loaders.py b/tests/test_loaders.py
index 97695c5..2447c1c 100644
--- a/tests/test_loaders.py
+++ b/tests/test_loaders.py
@@ -36,7 +36,7 @@ def test_cite():
         sys.stdout = sys.__stdout__
 
 
-KNOWN_ISSUES = {'gtzan_genre': ['all']}  # issue #242, remove when fixed
+KNOWN_ISSUES = {}  # key is module, value is REMOTE key
 
 
 def test_download(mocker):