Reworking chord detection to be order independent. #214
Conversation
| @@ -33,12 +37,41 @@ export function detect(source: string[]): string[] { | |||
| .map((chord) => chord.name); | |||
| } | |||
|
|
|||
| // Assumes that chord is presorted | |||
| function findRoot(chord: string[]): string { | |||
There was a problem hiding this comment.
Can you explain me why this works? 🙏
There was a problem hiding this comment.
This is shamelessly borrowed from this comment, which I think explains it better than I could.
Unfortunately this will only work on chords forming major or minor triads.
| function findExactMatches(notes: string[], weight: number): FoundChord[] { | ||
| const tonic = notes[0]; | ||
| const tonicChroma = note(tonic).chroma; | ||
| const root = findRoot(notes); |
There was a problem hiding this comment.
Is this name change on purpose? Can you elaborate the reasoning?
There was a problem hiding this comment.
This was changed to root since it is looking for a root of a chord and not the tonic of a scale.
| expect(detect(["E", "G#", "B", "C#"])).toEqual(["E6", "C#m7/E"]); | ||
| expect(detect(["D", "F#", "A", "C"])).toEqual(["D7/C"]); | ||
| expect(detect(["D3", "F#4", "A3", "C4"])).toEqual(["D7"]); | ||
| expect(detect(["F#4", "A3", "C4", "D3"])).toEqual(["D7"]); |
| }); | ||
|
|
||
| test("(regression) detect aug", () => { | ||
| expect(detect(["C", "E", "G#"])).toEqual(["Caug", "Eaug/C", "G#aug/C"]); | ||
| expect(detect(["E", "G#", "C"])).toEqual(["Caug", "Eaug/C", "G#aug/C"]); |
There was a problem hiding this comment.
Not sure about this change. For me E G# C is is Eaug (the base note is clearly E, not C)
There was a problem hiding this comment.
In this case since every note is in the same octave, would C not be the bass note?
My goal with this change was to allow callers to pass an unordered list of notes to be detected, but that also requires that all notes be explicitly assigned an octave.
|
@danigb I saw you closed this PR but am not sure what was missing here to get merged (except being stale). Could you kindly elaborate on that? |
Resolves #160.