Skip to content

Adding loader for MIR-1K - #649

Open
davidmarttila wants to merge 6 commits into
mir-dataset-loaders:masterfrom
davidmarttila:mir1k
Open

Adding loader for MIR-1K#649
davidmarttila wants to merge 6 commits into
mir-dataset-loaders:masterfrom
davidmarttila:mir1k

Conversation

@davidmarttila

Copy link
Copy Markdown
Contributor

Description

Please include the following information at the top level docstring for the dataset's module mydataset.py:

  • Describe annotations included in the dataset
  • Indicate the size of the datasets (e.g. number files and duration, hours)
  • Mention the origin of the dataset (e.g. creator, institution)
  • Describe the type of music included in the dataset
  • Indicate any relevant papers related to the dataset
  • Include a description about how the data can be accessed and the license it uses (if applicable)

Dataset loaders checklist:

  • Create a script in scripts/, e.g. make_my_dataset_index.py, which generates an index file.
  • Run the script on the canonical version of the dataset and upload the index to Zenodo Audio Data Loaders community.
  • Create a sample version of the index with the necessary information for testing.
  • Create a module in mirdata, e.g. mirdata/my_dataset.py
  • Create tests for your loader in tests/datasets/, e.g. test_my_dataset.py
  • Add your module to docs/source/mirdata.rst and docs/source/table.rst
  • Run black, flake8 and mypy (see Running your tests locally).
  • Run tests/test_full_dataset.py on your dataset.
  • Check that codecov coverage does not decrease.

Other recordings

The MIR-1K dataset is based on 110 song recordings split into 1000 clips.

The dataset download also includes a folder UndividedWavfile which contains the "un-split" recordings. Additionally, while all other annotation files are on the clip level, the download also includes a LyricsWav folder containing recordings of the song lyrics being read aloud (as speech, not singing), but these correspond to the 110 "un-split" songs, not the 1000 clips. So I wasn't quite sure how to handle these within the mirdata structures, for now I simply ignored them.

@codecov

codecov Bot commented Dec 13, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.12%. Comparing base (97717c6) to head (31390a3).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
+ Coverage   97.09%   97.12%   +0.03%     
==========================================
  Files          69       70       +1     
  Lines        7720     7801      +81     
==========================================
+ Hits         7496     7577      +81     
  Misses        224      224              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@genisplaja genisplaja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hey @dsuedholt, thanks for the contribution and sorry for the late review. I am leaving a few questions before accepting the PR, to hear your opinons and potentially opinions from peer maintainers.

Also, should be ignore the non-split portion of the dataset? Is it normally used or not used in related research works? I guess loading them would mean have a bunch of more tracks included in the dataset, but those would mostly be already present in the dataset in the split format right?

Thanks again for your work, I did also take a look at the upload request for the index in Zenodo and looks fine, so I am going to be accepting it :)

assert isinstance(all_data, dict)

mtrack_ids = dataset.mm
mtrack_ids = dataset.mtrack_ids

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good catch, and sorry about that. We are going to be adding a fix for that in a separate PR here: #657.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess that after mering PR #657, we can propagate the update here.

"""

return annotations.LyricData(
intervals=np.array([[0.0, 0.0]]),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was wondering if it was worth getting the duration of the track and having an interval like [0.0, 10.0], 10.0 being an example total duration of the track. Maybe the faster way would be to parse it from the annotations. However, I wonder if there's chance that someone needs lyrics+duration and no audio/annotations from where to get the duration also.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point - potentially this could also be a way to include the lyric data annotation with the unsplit files. i.e. assume a track is split into clips of 10 second lengths, then for the unsplit track, include the lyrics annotations for each clip with the intervals [0, 10], [10, 20] etc?

)


# not using @io.coerce_to_string_io here because it has hardcoded utf-8 encoding

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Interesting, noting this issue. We could have a tailored decorator later.


Cached Properties:
f0 (F0Data): the track's f0 annotation
lyrics (LyricData): the track's lyrics

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

maybe if we end up leaving the lyrics with no interval/duration information, we could also let the user know here?

@davidmarttila

Copy link
Copy Markdown
Contributor Author

Hi @genisplaja, thanks for the review!

Most research that I'm personally aware of (using the dataset for either pitch detection or source separation) does not use the unsplit tracks, because the annotations are given on a clip level. The only additional information that is only available on the unsplit track level are the spoken lyric recordings. But I agree that simply ignoring the unsplit tracks isn't a satisfactory solution, but I wasn't really sure what the best way to include them would be.

What do you think of the following approach?

  1. Add the unsplit tracks into the index
  2. Add an attribute unsplit_track_id or similar, that would be None for unsplit tracks, and the ID of the corresponding unsplit track for clips. That would make it easy for people to filter out whichever one they want to use
  3. (Potentially) stitching together the annotation timestamps of the clips to provide annotations for the unsplit track? Or does that stray too far from "just load the data as released"

@magdalenafuentes

Copy link
Copy Markdown
Collaborator

Hey @dsuedholt, thanks for this PR, it looks great! I have just one comment/suggestion. I noticed that you are using the Track object with instrumental and vocal audio. I would suggest that you use the MultiTrack object instead. My concern is that from the user's perspective, Track is expected to have a single attribute audio and it might be confusing, whereas MultiTrack is more flexible and can point to different audio by design (e.g. vocals, instrumental). Thoughts?

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