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

[WIP] Changes for Gtzan dataset #569

Draft
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

harshpalan
Copy link
Collaborator

@harshpalan harshpalan commented Dec 20, 2022

As discussed with @magdalenafuentes, I have made the changes for some issues and the gtzan dataset (dataset not available anymore).

One of these changes is adding optional dependencies to the test in setup.py so users can quickly test mirdata without facing any issues.

@genisplaja @nkundiushuti please review and let me know if any changes are required.

Fixes #568 , #562, #564, #549

@harshpalan harshpalan changed the title Changes for Gtzan dataset, fixes for 568,562,564,549. [WIP] Changes for Gtzan dataset, fixes for 568,562,564,549. Dec 20, 2022
from mirdata.datasets import compmusic_carnatic_rhythm
except ImportError:
raise ImportError(
"In order to use CompMusic Carnatic Music Rhythm you must have openpyxl installed. "
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would propose this just because the exception is a bit more general and it can fail in other ways rather than missing dependencies.
"An error occured when importing this dataset. Most likely this is due to a dependency not being installed, in this case openpyxl."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, I'll make the changes.

@@ -4,11 +4,10 @@
try:
import DALI
except ImportError:
logging.error(
raise ImportError(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, the same thing as above:
"An error occured when importing this dataset. Most likely this is due to "

@@ -4,11 +4,10 @@
try:
import music21
except ImportError:
logging.error(
raise ImportError(
Copy link
Collaborator

Choose a reason for hiding this comment

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

"An error occured when importing this dataset. Most likely this is due to "

@magdalenafuentes
Copy link
Collaborator

Let's fix #564 here as well

@magdalenafuentes magdalenafuentes changed the title [WIP] Changes for Gtzan dataset, fixes for 568,562,564,549. [WIP] Changes for Gtzan dataset Jan 27, 2023
@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #569 (14f8bd2) into master (496eb4a) will increase coverage by 0.05%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##           master     #569      +/-   ##
==========================================
+ Coverage   96.96%   97.02%   +0.05%     
==========================================
  Files          58       58              
  Lines        6990     6987       -3     
==========================================
+ Hits         6778     6779       +1     
+ Misses        212      208       -4     

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.

Tests are failing when openpyxl is not installed.
3 participants