Harmonic mix plugin #6241
Harmonic mix plugin #6241
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider normalizing the key string (e.g., uppercasing and/or handling a trailing "major"/"minor" suffix) in
get_compatible_keysso that common variations like"am"or"C major"still resolve correctly againstCIRCLE_OF_FIFTHS. get_bpm_rangeassumesbpmis numeric, but library items often store fields as strings; it might be safer to coercesource_bpmtofloatwith error handling before passing it toget_bpm_rangeto avoid runtime type errors.- When building the BPM query (
bpm:{min_b}..{max_b}), you may want to format or round the bounds to a reasonable precision (e.g., integers) to keep the query predictable and avoid edge cases with long float representations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider normalizing the key string (e.g., uppercasing and/or handling a trailing "major"/"minor" suffix) in `get_compatible_keys` so that common variations like `"am"` or `"C major"` still resolve correctly against `CIRCLE_OF_FIFTHS`.
- `get_bpm_range` assumes `bpm` is numeric, but library items often store fields as strings; it might be safer to coerce `source_bpm` to `float` with error handling before passing it to `get_bpm_range` to avoid runtime type errors.
- When building the BPM query (`bpm:{min_b}..{max_b}`), you may want to format or round the bounds to a reasonable precision (e.g., integers) to keep the query predictable and avoid edge cases with long float representations.
## Individual Comments
### Comment 1
<location> `test/test_harmonicmix.py:20-29` </location>
<code_context>
+from beetsplug.harmonicmix import HarmonicLogic
+
+
+def test_standard_compatibility():
+ """Verify standard Circle of Fifths relationships."""
+ # Case: C Major should contain G (Dominant), F (Subdominant), Am (Relative Minor)
+ keys = HarmonicLogic.get_compatible_keys("C")
+ assert "G" in keys
+ assert "F" in keys
+ assert "Am" in keys
+
+ # Case: Am should match Em
+ keys_am = HarmonicLogic.get_compatible_keys("Am")
+ assert "Em" in keys_am
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Cover additional edge cases for `get_compatible_keys`, such as whitespace and unknown keys.
Current tests cover valid keys and empty/None, but miss a few behaviors worth pinning down:
- Inputs with surrounding whitespace (e.g. `" C "`) should resolve correctly because of `.strip()`.
- Unknown/invalid keys (e.g. `"H#"`, `"foo"`) should return an empty list via `dict.get(key, [])`.
Consider adding tests like:
- `assert HarmonicLogic.get_compatible_keys(" C ") == HarmonicLogic.get_compatible_keys("C")`
- `assert HarmonicLogic.get_compatible_keys("H#") == []`
Suggested implementation:
```python
"""Tests for harmonicmix plugin. Tests only cover the logic class."""
from beetsplug.harmonicmix import HarmonicLogic
```
```python
def test_standard_compatibility():
"""Verify standard Circle of Fifths relationships."""
# Case: C Major should contain G (Dominant), F (Subdominant), Am (Relative Minor)
keys = HarmonicLogic.get_compatible_keys("C")
assert "G" in keys
assert "F" in keys
assert "Am" in keys
# Case: Am should match Em (relative compatibility)
keys_am = HarmonicLogic.get_compatible_keys("Am")
assert "Em" in keys_am
def test_whitespace_keys_are_equivalent():
"""Inputs with surrounding whitespace should resolve correctly."""
keys_plain = HarmonicLogic.get_compatible_keys("C")
keys_spaced = HarmonicLogic.get_compatible_keys(" C ")
assert keys_spaced == keys_plain
def test_unknown_keys_return_empty_list():
"""Unknown/invalid keys should return an empty list."""
assert HarmonicLogic.get_compatible_keys("H#") == []
assert HarmonicLogic.get_compatible_keys("foo") == []
```
</issue_to_address>
### Comment 2
<location> `test/test_harmonicmix.py:45-50` </location>
<code_context>
+ assert "E#" in keys_c
+
+
+def test_bpm_range_calculation():
+ """Verify the BPM range logic (+/- 8%)."""
+ # 100 BPM -> Range should be 92 to 108
+ min_b, max_b = HarmonicLogic.get_bpm_range(100, 0.08)
+ assert min_b == 92.0
+ assert max_b == 108.0
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `get_bpm_range` when `bpm` is 0 or `None`, and for the default range_percent.
`test_bpm_range_calculation` validates the formula with an explicit `range_percent`, but the function also has behaviors that should be locked in via tests:
- `bpm is None` (or other falsy values) should return `(0, 0)`, e.g. `assert HarmonicLogic.get_bpm_range(None) == (0, 0)`.
- If `bpm` being `0` is intentional, a test like `assert HarmonicLogic.get_bpm_range(0) == (0, 0)` documents that contract.
- A test that omits `range_percent` ensures the default remains 8%.
These will strengthen edge-case coverage for BPM handling.
Suggested implementation:
```python
def test_bpm_range_calculation():
"""Verify the BPM range logic (+/- 8%)."""
# 100 BPM -> Range should be 92 to 108
min_b, max_b = HarmonicLogic.get_bpm_range(100, 0.08)
assert min_b == 92.0
assert max_b == 108.0
def test_bpm_range_none_bpm():
"""bpm=None (or other falsy) should return (0, 0)."""
assert HarmonicLogic.get_bpm_range(None) == (0, 0)
def test_bpm_range_zero_bpm():
"""bpm=0 should return (0, 0) to document the contract."""
assert HarmonicLogic.get_bpm_range(0) == (0, 0)
def test_bpm_range_default_percent():
"""Omitting range_percent should use the default +/- 8%."""
min_b, max_b = HarmonicLogic.get_bpm_range(100)
assert min_b == 92.0
assert max_b == 108.0
```
These tests assume `HarmonicLogic.get_bpm_range` currently implements:
- A falsy check like `if not bpm: return (0, 0)`.
- A default `range_percent` of `0.08`.
If the implementation differs (e.g., stricter type checks or a different default), you should align either the implementation or the expected values in these tests accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_bpm_range_calculation(): | ||
| """Verify the BPM range logic (+/- 8%).""" | ||
| # 100 BPM -> Range should be 92 to 108 | ||
| min_b, max_b = HarmonicLogic.get_bpm_range(100, 0.08) | ||
| assert min_b == 92.0 | ||
| assert max_b == 108.0 |
There was a problem hiding this comment.
suggestion (testing): Add tests for get_bpm_range when bpm is 0 or None, and for the default range_percent.
test_bpm_range_calculation validates the formula with an explicit range_percent, but the function also has behaviors that should be locked in via tests:
bpm is None(or other falsy values) should return(0, 0), e.g.assert HarmonicLogic.get_bpm_range(None) == (0, 0).- If
bpmbeing0is intentional, a test likeassert HarmonicLogic.get_bpm_range(0) == (0, 0)documents that contract. - A test that omits
range_percentensures the default remains 8%.
These will strengthen edge-case coverage for BPM handling.
Suggested implementation:
def test_bpm_range_calculation():
"""Verify the BPM range logic (+/- 8%)."""
# 100 BPM -> Range should be 92 to 108
min_b, max_b = HarmonicLogic.get_bpm_range(100, 0.08)
assert min_b == 92.0
assert max_b == 108.0
def test_bpm_range_none_bpm():
"""bpm=None (or other falsy) should return (0, 0)."""
assert HarmonicLogic.get_bpm_range(None) == (0, 0)
def test_bpm_range_zero_bpm():
"""bpm=0 should return (0, 0) to document the contract."""
assert HarmonicLogic.get_bpm_range(0) == (0, 0)
def test_bpm_range_default_percent():
"""Omitting range_percent should use the default +/- 8%."""
min_b, max_b = HarmonicLogic.get_bpm_range(100)
assert min_b == 92.0
assert max_b == 108.0These tests assume HarmonicLogic.get_bpm_range currently implements:
- A falsy check like
if not bpm: return (0, 0). - A default
range_percentof0.08.
If the implementation differs (e.g., stricter type checks or a different default), you should align either the implementation or the expected values in these tests accordingly.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6241 +/- ##
==========================================
- Coverage 68.26% 68.18% -0.08%
==========================================
Files 138 139 +1
Lines 18791 18846 +55
Branches 3167 3177 +10
==========================================
+ Hits 12827 12851 +24
- Misses 5290 5321 +31
Partials 674 674
🚀 New features to boost your workflow:
|
|
I think I'm finished with the code and docs, but the documentation linter keeps failing on the CI server even though it passes locally on my Windows machine. I made some attempts but I cannot pinpoint what's the problem. Could you help me spot the culprit? |
You tried those https://beets.readthedocs.io/en/latest/contributing.html#style |
|
But first of all: Welcome! Thanks for the contribution. I've always wanted to add circle of fiths functionality to Beets but I was picturing it a little differently, the main point being to be able to use it as a general sorting mechanism so it's usable with the |
Hello! Thank you for replying so quickly and for welcoming me!
To conclude, I'll wait for your full review and then start the refactor. Thanks again and happy holidays! |
|
Hi Angelos, I see two use cases here:
Sorting
|
2 participants
Description
This PR adds a new plugin, harmonicmix, which calculates the harmonic compatibility between tracks. Playlist curators may use this to offer more harmonically coherent lists. DJs may use this to search for compatible songs around the same bpm as their current track.
This is my first contribution to Beets! I really enjoy using the tool and wanted to give back using my music theory knowledge. I hope this is helpful, and I am happy to make any changes needed to get this merged. My coding skills are not that sharp, so any feedback is much appreciated! I tried to follow community standards, but please let me know if I missed anything.
Features:
Harmonic Matching: Finds tracks compatible with the source song's key using the Circle of Fifths. For example, it is easy to move from a song in C to a song in F, G, or Am. It is really difficult to move to a song in G#.
Enharmonic Support: Handles standard keys (e.g., C# and Db are treated as the same), as well as more nuanced options (like E#).
BPM Filtering: Limits results to a mixable BPM range (+/- 8%).
Future Improvements
Make the BPM range configurable (currently fixed at +/- 8%).
Integrate calls to autobpm and keyfinder to fill in missing metadata.
To Do