-
Notifications
You must be signed in to change notification settings - Fork 60
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 MDB-stem-synth dataset #639
Adding loader for MDB-stem-synth dataset #639
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #639 +/- ##
==========================================
+ Coverage 97.05% 97.06% +0.01%
==========================================
Files 66 67 +1
Lines 7498 7537 +39
==========================================
+ Hits 7277 7316 +39
Misses 221 221 |
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.
Hey @dsuedholt, great PR, very neat. Thanks for that! I am leaving few minor comments to make the docs render properly and to issue a small linting bug but LGTM.
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.
LGTM :)
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.
hello again @dsuedholt :) Thanks for your time. Once this minor thing is done, I think we can merge :) If you have the time, could you please do it soon so that the loader can be included in the release we have scheduled for this week? Thanks!!
Thanks for the friendly and helpful review @genisplaja! Let me know if there's anything else you need for the release. |
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 think now this is ready to merge :) Thanks @dsuedholt for the contributing and the timely responses!
Description
Please include the following information at the top level docstring for the dataset's module mydataset.py:
Dataset loaders checklist:
scripts/
, e.g.make_my_dataset_index.py
, which generates an index file.mirdata/indexes/
e.g.my_dataset_index.json
.mirdata/my_dataset.py
tests/datasets/
, e.g.test_my_dataset.py
docs/source/mirdata.rst
anddocs/source/table.rst
black
,flake8
andmypy
(see Running your tests locally).tests/test_full_dataset.py
on your dataset.