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

Adding loader for CIPI dataset #599

Merged
merged 50 commits into from
Nov 2, 2023
Merged

Adding loader for CIPI dataset #599

merged 50 commits into from
Nov 2, 2023

Conversation

PRamoneda
Copy link
Collaborator

@PRamoneda PRamoneda commented Oct 30, 2023

Adding loader for CIPI (Can I play it?)

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

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 save the index in mirdata/indexes/ e.g. my_dataset_index.json.
  • 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 tests/test_full_dataset.py on your dataset.

@guillemcortes guillemcortes added the new loader request to add a specific dataset loader label Oct 31, 2023
Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Merging #599 (0dad936) into master (496eb4a) will increase coverage by 0.06%.
Report is 8 commits behind head on master.
The diff coverage is 98.06%.

@@            Coverage Diff             @@
##           master     #599      +/-   ##
==========================================
+ Coverage   96.96%   97.02%   +0.06%     
==========================================
  Files          58       60       +2     
  Lines        6990     7133     +143     
==========================================
+ Hits         6778     6921     +143     
  Misses        212      212              

Copy link
Collaborator

@genisplaja genisplaja left a comment

Choose a reason for hiding this comment

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

Hey @PRamoneda, thanks for that! Just adding some questions/comments and minor fixes to do. Let me know if you have any questions!

docs/source/mirdata.rst Outdated Show resolved Hide resolved
mirdata/datasets/cipi.py Outdated Show resolved Hide resolved
mirdata/datasets/cipi.py Outdated Show resolved Hide resolved
mirdata/datasets/cipi.py Outdated Show resolved Hide resolved
tests/test_loaders.py Outdated Show resolved Hide resolved
mirdata/datasets/cipi.py Outdated Show resolved Hide resolved
mirdata/datasets/cipi.py Outdated Show resolved Hide resolved
Comment on lines 183 to 187
except FileNotFoundError:
raise FileNotFoundError(
"MusicXML file {} for track {} not found. "
"Did you run .download()?".format(self.musicxml_paths, self.track_id)
)
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 this message is not well written! Because if a music xml file is not available, load_score will fail and specify which file is missing, but here, you will print the entire list... maybe you could just say something like "Missing MusicXML files. Did you run .download()?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hey @PRamoneda sorry we missed that one. Can you take a look again?

mirdata/datasets/cipi.py Show resolved Hide resolved
@PRamoneda
Copy link
Collaborator Author

It is ready! I am free. If you need help with something, let me know. And see you at ISMIR!

@genisplaja genisplaja self-requested a review November 2, 2023 14:44
Copy link
Collaborator

@genisplaja genisplaja left a comment

Choose a reason for hiding this comment

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

Thanks @PRamoneda! Added a few minor comments for the Track docstring but other than that looks good to me :) Please fix the mentioned mini-issues and IMO the loader will be ready to go!

mirdata/datasets/cipi.py Outdated Show resolved Hide resolved
mirdata/datasets/cipi.py Outdated Show resolved Hide resolved
@guillemcortes guillemcortes self-requested a review November 2, 2023 17:48
@guillemcortes guillemcortes merged commit c1dccb0 into master Nov 2, 2023
9 checks passed
@guillemcortes guillemcortes deleted the pedro/cipi branch November 2, 2023 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new loader request to add a specific dataset loader
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants