-
Notifications
You must be signed in to change notification settings - Fork 223
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
Reworking chord detection to be order independent. #214
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
import { all } from "@tonaljs/chord-type"; | ||
import { note } from "@tonaljs/core"; | ||
import { note, distance } from "@tonaljs/core"; | ||
import { name, sortedNames } from "@tonaljs/note"; | ||
import { simplify } from "@tonaljs/interval"; | ||
import { modes } from "@tonaljs/pcset"; | ||
|
||
interface FoundChord { | ||
|
@@ -20,7 +22,9 @@ const namedSet = (notes: string[]) => { | |
}; | ||
|
||
export function detect(source: string[]): string[] { | ||
const notes = source.map((n) => note(n).pc).filter((x) => x); | ||
const notes = sortedNames(source) | ||
.map((n) => note(n).pc) | ||
.filter((x) => x); | ||
if (note.length === 0) { | ||
return []; | ||
} | ||
|
@@ -33,12 +37,41 @@ export function detect(source: string[]): string[] { | |
.map((chord) => chord.name); | ||
} | ||
|
||
// Assumes that chord is presorted | ||
function findRoot(chord: string[]): string { | ||
let foundRoot = null; | ||
chord.every((note) => { | ||
const workComplete = chord.some((otherNote) => { | ||
const interval = simplify(distance(note, otherNote)); | ||
const orderedNotes = sortedNames([note, otherNote]); | ||
if (interval === "5P") { | ||
foundRoot = orderedNotes[0]; | ||
return foundRoot; // Loop is complete | ||
} else if (interval === "4P") { | ||
foundRoot = orderedNotes[1]; | ||
return foundRoot; // Loop is complete | ||
} | ||
return false; // continue looping | ||
}); | ||
// Continue looping if the root note was not found | ||
return !workComplete; | ||
}); | ||
if (foundRoot) { | ||
return foundRoot; | ||
} else { | ||
// Defaults to the old behavior if the chord is complex and the root note cannot be easily found | ||
return chord[0]; | ||
} | ||
} | ||
|
||
// assumes that notes is presorted | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this name change on purpose? Can you elaborate the reasoning? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was changed to root since it is looking for a root of a chord and not the tonic of a scale. |
||
const noteName = namedSet(notes); | ||
// we need to test all chormas to get the correct baseNote | ||
// we need to test all chromas to get the correct baseNote | ||
const allModes = modes(notes, false); | ||
const baseNote = notes[0]; | ||
const baseChroma = note(baseNote).chroma; | ||
|
||
const found: FoundChord[] = []; | ||
allModes.forEach((mode, index) => { | ||
|
@@ -47,15 +80,16 @@ function findExactMatches(notes: string[], weight: number): FoundChord[] { | |
|
||
chordTypes.forEach((chordType) => { | ||
const chordName = chordType.aliases[0]; | ||
const baseNote = noteName(index); | ||
const isInversion = index !== tonicChroma; | ||
const rootNote = noteName(index); | ||
const isInversion = note(rootNote).chroma !== baseChroma; | ||
|
||
if (isInversion) { | ||
found.push({ | ||
weight: 0.5 * weight, | ||
name: `${baseNote}${chordName}/${tonic}`, | ||
name: `${rootNote}${chordName}/${baseNote}`, | ||
}); | ||
} else { | ||
found.push({ weight: 1 * weight, name: `${baseNote}${chordName}` }); | ||
found.push({ weight: 1 * weight, name: `${root}${chordName}` }); | ||
} | ||
}); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,8 @@ | |
"dependencies": { | ||
"@tonaljs/chord-type": "^3.6.0", | ||
"@tonaljs/core": "^3.5.4", | ||
"@tonaljs/interval": "^3.5.4", | ||
"@tonaljs/note": "^3.5.4", | ||
"@tonaljs/pcset": "^3.5.4" | ||
}, | ||
"author": "[email protected]", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,14 +2,20 @@ import { detect } from "./index"; | |
|
||
describe("@tonal/chord-detect", () => { | ||
test("detect", () => { | ||
expect(detect(["D", "F#", "A", "C"])).toEqual(["D7"]); | ||
expect(detect(["F#", "A", "C", "D"])).toEqual(["D7/F#"]); | ||
expect(detect(["A", "C", "D", "F#"])).toEqual(["D7/A"]); | ||
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"]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice one!! 👏 |
||
expect(detect(["F#2", "A3", "C4", "D3"])).toEqual(["D7/F#"]); | ||
expect(detect(["A3", "C4", "D3", "F#4"])).toEqual(["D7"]); | ||
expect(detect(["A2", "C4", "D3", "F#4"])).toEqual(["D7/A"]); | ||
expect(detect(["E3", "G#4", "B4", "C#4"])).toEqual(["E6", "C#m7/E"]); | ||
expect(detect(["C4", "E4", "G4"])).toEqual(["CM", "Em#5/C"]); | ||
expect(detect(["E4", "G4", "C5"])).toEqual(["Gm#5", "CM/E"]); | ||
}); | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about this change. For me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
}); | ||
|
||
test("edge cases", () => { | ||
|
Large diffs are not rendered by default.
Large diffs are not rendered by default.
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.
Can you explain me why this works? 🙏
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.
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.