-
Notifications
You must be signed in to change notification settings - Fork 59
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 Filosax #536
Conversation
Hi mirdata team, I'm struggling to pass some of the tests, despite my best efforts… Firstly, there is failure in Cheers, Dave |
Hi Dave, thanks for the PR! We'll do our best to take a look in the next day or two, to have it ready by the end of the ISMIR tutorials. |
@dave-foster quick question - in the download info, you mention a script to run:
I had a quick look at the conversation in the issues about this - where does this script live now? One option is to point to e.g. another github repo of yours with the script. Another is to create a subfolder in mirdata, e.g. |
Hi Rachel, thanks for looking at this - I'm reminded of what a bummer it is that we aren't all making our way to Bangalore to be able to chat in person! The script that you mention is in the material that users download from the Zenodo site (see a couple of lines earlier for the "Lite" link). This works fine as a pre-processing step, so could stay like that, or be incorporated into mirdata, whichever works best. Should I upload the script somewhere to see what you reckon? I would point out that if any of this gives the impression that I have the faintest idea what I'm doing, then please don't be deceived! |
Hi @rabitt et al, a friendly bump on this PR, to see if anybody could help me out in getting it approved? I've returned to the code this week to make some updates, and it turns out that I still haven't acquired the skills to get it through the unit tests! |
hi! could we help you merge this? I think it currently fails. |
Thanks Marius, I'd kind of given up hope of it ever getting merged, so any help you could give would be greatly appreciated! |
ok if you could add the preprocessing script, take a look at the missing tests and the errors in circleci then I can take a look |
Thanks Marius, the preprocessing script is in there already, and the whole reason that I haven't previously been able to get it merged is that I don't understand exactly why it is failing the circleci tests! I seem to remember trying a whole bunch of things to get it to work, but being thwarted at every attempt. |
if you could try another commit and push then I could check. I am unable to see the errors at the moment . also..what errors do you get when running the tests locally? is everything ok there? |
Hello @dave-foster! I think you are forgetting to indicate, in the
and then have a function to load the notes like that:
or maybe unifying this function with the Hope this helps!! |
Codecov Report
@@ Coverage Diff @@
## master #536 +/- ##
==========================================
+ Coverage 96.61% 96.64% +0.02%
==========================================
Files 49 49
Lines 6004 6106 +102
==========================================
+ Hits 5801 5901 +100
- Misses 203 205 +2 |
By Jove, he's got it! Brilliant, thanks @genisplaja. I had no idea I needed to add the info to the I'm left now with a fail on the |
Great @dave-foster, sorry for the delay in finding that out :) See the following example:
In this case, an additional testing example is loaded here to test the case when a particular annotation is not available for certain tracks. You can just leave the default testing track in test_loaders.py as it is now, and momentarily load an additional one to cover the untested lines. No need to include the additional track in test_loaders.py . If I'm not mistaken, it should work as the example I attached.
Let me know if you have further questions! |
Finally passed all tests! Excellent guidance, @genisplaja - your example showed me what I needed to do in the test file. In the end, I combined it all into the |
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.
Hi @dave-foster! Just a small set of easy-to-address changes! The loader looks very good to me :) I'm just requesting small fixes, mainly related to code formatting and docs, and in my opinion, is ready to merge!!
Just a small thing. I think that if you click on this link you'll be able to read the docs that are generated by this PR. Let me know if that doesn't work. Then you can go to filosax
in Dataset Loaders
in the menu and check that everything looks and reads nicely there!
Thanks again for your patience and timely responses. The loader looks really nice!! Let me know if you have further questions.
Hello @dave-foster! Did you see my small review? Do you need help with this? Thanks!! |
Hi @genisplaja, thanks for the nudge, and apologies for the delay getting your prescribed fixes incorporated into the code now. I've addressed all of the issues that you mention, and have added comments to those where I'm not quite sure how to proceed. Let me know what I need to do from this point? I can't tell you how much I appreciate your intervention to get this over the line! |
No problem @dave-foster! Happy to help :) This looks almost ready, I just gave a response to the open comments in my last review, and I observed in the docs the following issue: File: |
Thanks again @genisplaja - I think that's everything covered now! |
I just left a tiny comment but looks ready to me. I'll ping @nkundiushuti (and whoever else is interested) to take a look and we can merge this. Thank you very much for your work and patience @dave-foster! |
""" | ||
|
||
|
||
class Note: |
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.
this is not the ideal way of implementing this but I am fine with it. we have a an annotation class and a NoteData class inside annotations https://github.com/mir-dataset-loaders/mirdata/blob/master/mirdata/annotations.py
ideally this should have been inherited Annotation. I suggest going on with this PR and opening another issue regarding this (which can be solved in the future).
thanks @dave-foster . I left some comment regarding the Note class which I think could have been implemented using the existing functionalities. however I am happy with it and I think it can be merged |
Brilliant, huge thanks @nkundiushuti and @genisplaja! I'll certainly have a look into conforming the |
All checkboxes checked!