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] Implement current MIREX key scoring method and warn if using old one #339

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stefan-baumann
Copy link

This PR aims to solve the issue discussed in #337 in a backwards-compatible manner.
An additional argument allow_descending_fifths was added to the key.weighted_score and key.evaluate methods, which is set to False by default (and thus being backwards-consistent), but raises a warning stating that this method is no longer used for MIREX. If set to True, descending fifth errors are given a score of 0.5, matching current MIREX scoring.

I'd love to get feedback on whether the maintainers approve of the way I implemented this change and whether they might have ideas how to improve keyword naming or the added documentation.

If the feedback is positive, I'll extend the tests to cover this behaviour as well.

Copy link
Collaborator

@craffel craffel left a comment

Choose a reason for hiding this comment

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

I think this is a reasonable approach, but we should plan to switch the default to True eventually if it matches the new convention.

@craffel
Copy link
Collaborator

craffel commented Jul 23, 2021

@bmcfee @jpauwels @iansimon PTAL

@stefan-baumann
Copy link
Author

stefan-baumann commented Jul 23, 2021

but we should plan to switch the default to True eventually if it matches the new convention

In that case, we should probably change the warning message to reflect that. Maybe something like this?

The selected key scoring method does not match that currently used by MIREX. To use the same method, specify allow_descending_fifths=True. The default behaviour will change to allow_descending_fifths=True in the future.

Potentially substituting in the future with a specific version number.

@craffel
Copy link
Collaborator

craffel commented Jul 23, 2021

but we should plan to switch the default to True eventually if it matches the new convention

In that case, we should probably change the warning message to reflect that. Maybe something like this?

The selected key scoring method does not match that currently used by MIREX. To use the same method, specify allow_descending_fifths=True. The default behaviour will change to allow_descending_fifths=True in the future.

Potentially substituting in the future with a specific version number.

SGTM

@stefan-baumann stefan-baumann force-pushed the update_key_eval_method branch from f114387 to 31729d8 Compare July 23, 2021 17:45
@stefan-baumann
Copy link
Author

Okay, I added the note to the warning and the docstring, specifying that the default behaviour will change in the future.

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.

2 participants