ChordDetect is incorrectly detecting chord inversions

[sai-chai]

First off, this package is great and really versatile. I'd love to contribute sometime!

That said:
Using ChordDetect, I found that the order of notes in a collection changes the outcome, showing chord inversions even though the actual octave values of the notes being played rule that out. For now, I'm using Note.sortedNames to fix this issue, but it would be good to have the library itself detect inversions using octaves instead of collection order.

Here's a screencap showing how a different order of notes results in a fundamentally different set of chords.

[danigb]

Thanks for reporting! Can you provide some code examples, please? That makes me easy to find the problem (and the solution)

[lukephills]

I don't see a difference when changing the order of notes in the collection however inversions can be quite weird. Take this example:

// First returned chord == "CM" (this is correct)
Chord.detect(["C4","E4","G4"]); 

// First matching chord == "Em#5" when it should be "CM/E"
Chord.detect(["E4","G4", "C5"]); 

It seems to me that the CM/E should be the preferred chord here.

[sai-chai]

@lukephills The chord types could be ranked by how common they are, but unfortunately there's no data or convention to inform that ranking, and a naming convention that's common to one genre might not be used by another. Rock is a perfect example b/c power chords are sort-of ignored by other genres.

[sai-chai]

@danigb The example I have is a bit complicated.

const [currentNotes, setCurrentNotes] = React.useState([]);

   const playNote = React.useCallback(
      midiNote => () => {
         if (!activeNodes[midiNote]) {
            audioContext.resume().then(() => {
               setActiveNodes({
                  ...activeNodes,
                  [midiNote]: instrument.play(midiNote),
               });
            });
            setCurrentNotes(currentNotes.concat(Note.fromMidi(midiNote)));
         }
      },
      [audioContext, instrument, activeNodes],
   );

In this block, assume the currentNotes collection hasn't been sorted:

   const currentChord = React.useMemo(
      () => Chord.detect(Note.sortedNames(currentNotes)),
      [currentNotes],
   );

Here's the full file that I've pulled these blocks from:
https://github.com/sai-chai/choordinator/blob/200d0ae8bd35c504c3e1506137cf6248fa44964e/src/containers/PianoRoll/index.js#L123

[danigb]

Let me see if I understood: the problem is that same notes in different order produces same chords in different order... If so, I think this is on purpose: chords without inversions are listed first, despite how common the chord is...

Anyway, I'm open to any idea. What should be, in your opinion, the expected output?

Thanks for the explanation

[sai-chai]

The order of chords isn't much of an issue. Like I said, it relies on naming conventions that are variable depending on context.

The real issue is that the order of notes in a collection, given constant octave values, has an effect on what chord the function detects. This shouldn't be the case b/c no matter the collection order, the intervals are identical b/c they're the exact same notes.

If you go back to the screencap I attached in the first post, the chords it's detecting aren't identical. [Fmaj, Am#5/F] and [Am#5, Fmaj/A] aren't the same chords, and more importantly, only one of these is the correct (Fmaj, Am#5/F).

A3, C4, F3 can be named either Fmaj or Am#5/F, not Fmaj/A or Am#5, b/c the bass note is F3. However if you provide them to chord detect in that order, you get the wrong set of names. If you order these notes from lowest to highest (F3, A3, C4), only then does Chord.detect return the correct chord names.

[danigb]

Now I understand, thanks! 👍 I'll take a look and test those cases to see what I able to do

[sai-chai]

Much thanks!

[jhmcstanton]

@danigb I took a crack at this. The root note determination in there wont catch all chords, but I think it is order independent now. There is a fallback path to just using the lowest note as the root, which is similar to the current implementation but should also be order independent.

Let me know if you want any changes to it and I'll take another look.