-
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
Add SALAMI functions annotations and make included JAMS export be MSAF compatible #589
base: master
Are you sure you want to change the base?
Add SALAMI functions annotations and make included JAMS export be MSAF compatible #589
Conversation
Hmm, how could I get a review on this? Ping @rabitt, @magdalenafuentes do you have time to spare? |
Hey @carlthome, thanks for this PR and sorry for the slow response on our side. We've recently migrated soundata to GitHub actions and updated Python and packages version, and we're looking into doing the same in mirdata in the following couple of weeks, so we're holding a bit on the other PRs for the moment to incorporate those changes first and then have the PRs tested with the updated pipeline. We'll make sure to look at your PRs as soon as we finish that up so you'll hear from us soon. Thanks for your patience! |
For compatibility with MSAF (`msaf.eval.process`), each section should be a separate annotation instead of treating annotations as overlapping segments within one annotation.
1e7a1b3
to
e8d3ffa
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #589 +/- ##
==========================================
- Coverage 97.09% 97.07% -0.02%
==========================================
Files 62 62
Lines 7286 7309 +23
==========================================
+ Hits 7074 7095 +21
- Misses 212 214 +2 |
Think this is ready to go now but technically this is a user breaking change so I'm a bit worried and would like some guidance on how to land this. |
Awesome @carlthome ! Will try to take a look at it the next week. Thanks again! |
Any updates on this? |
Hi @carlthome ! Sorry for the radio silence, we have this on the radar and we will have a response next week for sure. Thanks for your patience! |
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 @carlthome , thanks for your patience. I'm suggesting some minor changes to make the section units universal rather than dataset-specific. Thanks again!
@@ -55,7 +55,12 @@ | |||
} | |||
|
|||
#: Section units | |||
SECTION_UNITS = {"open": "no scrict schema or units"} | |||
SECTION_UNITS = { |
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.
We would like to avoid dataset-specific units, so we suggest changing the name and the description to a more general ones. We propose the following although we are open to other options so feel free to propose them.
"salami_function": "Segment annotations with functional labels from the SALAMI guidelines" --> functional: Segment annotations with functional labels. E.g. intro, verse, chorus.
"salami_upper": "Segment annotations with SALAMI’s upper-case (large) label format" --> alphabetical_upper: Segment annotations with alphabetical upper-case (high-level) format. E.g. ABCA
"salami_lower": "Segment annotations with SALAMI’s lower-case (small) label format" --> alphabetical_lower: Segment annotations with alphabetical lower-case (low-level) format. E.g. aa’bb’ ccd bb’ aa’bb’ (ABCA)
Hi @carlthome , we've continued discussing this with the team and we think that we're ready to merge this once the suggested changes are made. |
Hi @carlthome, I hope you are doing well. |
This branch works with MSAF (which expects JAMS files and calls mir_eval internally in a joblib loop):