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

Key evaluation updates #396

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

bscherrer
Copy link

  • separate identification of key error category and score computation: error_scores are now a class variable of the KeyEvaluation class

  • addition of a new type of error category: relative_of_fifth that plays well with the strict_fifth option

  • additional documentation for the error_type function

  • updates to the key evaluation test code and data files to account for the separation of the error category determination and the score to attribute to a given key error

This pull request fixes #395 .

@superbock superbock requested a review from fdlm December 1, 2018 12:05
@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #396 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #396      +/-   ##
==========================================
+ Coverage    94.6%   94.65%   +0.04%     
==========================================
  Files          93       93              
  Lines       15078    15197     +119     
==========================================
+ Hits        14265    14384     +119     
  Misses        813      813
Impacted Files Coverage Δ
tests/test_utils.py 100% <ø> (ø) ⬆️
tests/test_evaluation_key.py 100% <100%> (ø) ⬆️
madmom/evaluation/key.py 96.55% <100%> (+2.52%) ⬆️
tests/test_bin_evaluate.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d33863...ec67a5f. Read the comment docs.

@bscherrer
Copy link
Author

bscherrer commented Jul 17, 2019

Hi @fdlm (or @superbock),

Would you mind having a look at this PR ? It should be quick :)

It does preserve previous behaviour and adds functionalities that can be useful to other researchers interested in musical key detection evaluation.

If there are changes you think are needed I would be happy to discuss.

Cheers,

Thanks !

Copy link
Contributor

@fdlm fdlm left a comment

Choose a reason for hiding this comment

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

Thanks for your work and sorry for the late reply. Your PR looks good, I'd like to request just some minor changes, then we're ready to go!

madmom/evaluation/key.py Outdated Show resolved Hide resolved
madmom/evaluation/key.py Outdated Show resolved Hide resolved
madmom/evaluation/key.py Outdated Show resolved Hide resolved
'Relative: {:.3f} Parallel: {:.3f} Other: {:.3f}'.format(
self.name, self.weighted, self.correct, self.fifth,
self.relative, self.parallel, self.other))
'Relative: {:.3f} Relative of Fifth: {:.3f} '
Copy link
Contributor

Choose a reason for hiding this comment

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

We should only report "Relative of Fifth" here if it is actually present in all the evaluation objects.

Copy link
Author

Choose a reason for hiding this comment

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

OK

madmom/evaluation/key.py Show resolved Hide resolved
@superbock
Copy link
Collaborator

What's the status of this PR? I'm preparing v0.17 and wonder if this is ready to be included.

@bscherrer
Copy link
Author

I guess I have missed the v0.17 release boat, sorry ... it should be done today (Montreal time zone).

@superbock
Copy link
Collaborator

No hurry, there's still some more time until the release will be ready. I was just wondering abot the current status.

@bscherrer
Copy link
Author

bscherrer commented Oct 25, 2019 via email

@bscherrer
Copy link
Author

@superbock and @fdlm all should be good now.

Copy link
Contributor

@fdlm fdlm left a comment

Choose a reason for hiding this comment

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

Good work! Looking forward to having this merged. Just two comments to polish the code until we can include it.

Also, we should squash some of the commits before/when merging this.

madmom/evaluation/key.py Outdated Show resolved Hide resolved
madmom/evaluation/key.py Outdated Show resolved Hide resolved
@superbock
Copy link
Collaborator

Ok, v0.17 was delayed a bit ;), but I'm currently working on getting a new version out, so I'd like to check back on this PR and its current status. @bscherrer can you comment on the two comments above? Would be really nice to have this in!

@bscherrer
Copy link
Author

Hi @superbock, I will get this done this week or next. Is that OK timewise ?

@bscherrer bscherrer requested a review from fdlm January 10, 2022 20:43
Copy link
Contributor

@fdlm fdlm left a comment

Choose a reason for hiding this comment

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

Looking good, thanks!

@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/codeql-analysis.yml:analyze/language:python. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

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.

Proposed updates to key evaluation
3 participants