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

Unexpected results from Chord.get() ? #155

Open
honkskillet opened this issue Mar 29, 2020 · 15 comments
Open

Unexpected results from Chord.get() ? #155

honkskillet opened this issue Mar 29, 2020 · 15 comments

Comments

@honkskillet
Copy link
Contributor

I am working on a project using this (awesome!) library. Sometime Chord.get() gives unexpected results. For example if the arguement is "C7", I get the expected,

{ name": "C dominant seventh", "aliases": [ "7", "dom" ], "type": "dominant seventh", "tonic": "C", "notes": [ "C", "E", "G", "Bb" ] ,...}

but if I enter "C2"

{ "name":"C2 , "aliases":["M",""], "type":"major", "tonic":"C2", "notes":["C2","E2","G2"] }

I see what is happening here. It is giving me the C major scale in a certain octave (the 2nd octive). But does it make sense for it to return these value? After all C5, C6 and C7 cannot (and should not) be used to look up the C major chord in those respective octaves. They represent other chords. BTW, "C37" => {"notes":["C37","E37","G37"]}. That's probably not useful.

Also an oddball thatI came across while investigating is "C4". This returns a quartile voicing, which is fine. But the "name" is just C and there is no name. Seems like it could get confused with a C major chord. And the quartile voicing isn't listed as a possible chord type if you call ChordType.names(). Are there other arguments to Chord.get that will return objects similar to the quartile voicing? Is there a way to see what they are?

{ "name": "C ", "aliases": [ "4", "quartal" ], "type": "", "tonic": "C", "notes": [ "C", "F", "Bb", "Eb" ] }

@honkskillet
Copy link
Contributor Author

Looking at the source, I see that the "quartal" chord comes from a (rather large) set of chords labelled "legacy" in chord-type/data.ts. All of the legacy chords have names set to empty string, which is why I assume they don't show up for ChordType.names().

@honkskillet
Copy link
Contributor Author

One suggestion for naming. For the names of the chord I would spell them out in full english. For example, the name of C7sus4 is currently "C suspended 4th seventh". Having both the shorthand "4th" and the spelled out "seventh" seems a little inconsistent. I would have it be "C suspended fourth seventh". Also for the chords with flat or sharps I would write them out, ie "C major seventh flat six" instead of "major seventh b6". I would be happen to do a pull request if you'd like.

@danigb
Copy link
Collaborator

danigb commented Mar 29, 2020

Hi @honkskillet

Glad you find Tonal useful 👍

I think you're right in all the points. First, I'm aware of the inconsistencies of Chord.get results. I think the best solution is to suppose that chord never uses octaves in its names. So Chord.get('C2') will return an empty chord. But this is a breaking change. What do you think?

On the naming, you're also right. A PR will be awesome 🎉

@honkskillet
Copy link
Contributor Author

Yes, I think 'C2' should return an empty chord object. It would be a breaking change but I'd be surprise if many (any?) people were using it for that purpose since 'C4' thru 'C7' don't return major chords.

Looking through the list of 'legacy' chords, most of them (>90%) could be handled by a relative simple parser that both reads the symbols and generate a 'name'. (These things can get a little complicated though when handling edge cases and ambiguities.)

@honkskillet
Copy link
Contributor Author

An example of an ambiguous chord would be C#9. Is that a Major 9 chord where C# is the root? Or a Major 7 chord with an add #9?

@honkskillet
Copy link
Contributor Author

BTW, currently 'C##9' yields
{"type": "dominant ninth", "notes": [ "C##", "E##", "G##", "B#", "D##" ], ...}. Due to the ambiguities already inherent in chord naming it might be best to disallow double sharps and double flats?

Also I see you have already grappled with some of the ambiguities inherent in chord naming, for example, from data.ts

"m9#5" yields ["1P 3m 6m 7m 9M"]
but
"m9b5" yield ["1P 2M 3m 5d 7m" ]

Neither is wrong or right. It'd probbably be good to choose one convention over the other (say have m9 add a 9M note to then end of a minor seventh chord) and then use something else, like for example 'add2' if you want to add that note an octave lower.

@honkskillet
Copy link
Contributor Author

I might be able to help with a parser, if that is what you want. If so, where in you code would you want it?

@danigb
Copy link
Collaborator

danigb commented Mar 30, 2020

Hi,

thanks for all comments! Try to answer 👇

An example of an ambiguous chord would be C#9 (and C##9)

Agree. I still think the best solution is to change Chord.get to receive (type, tonic) and remove any ambiguity

"m9#5" yields ["1P 3m 6m 7m 9M"]

I think that's an error. It should be: 1P 3m 5A 7m 9M. What do you think?

I might be able to help with a parser, if that is what you want.

Well, not 100% sure. The good side of having a dictionary is that "for the same price" you can "detect" chord names using pitch class sets. So probably, not only a parser but a "detector/generator" (the reverse process: convert from intervals to chord names) would be nice.

So, from my side, if you want to start the adventure, go ahead! Good news. It would be a great addition or replacement of the current system 👍

Maybe you can build a first version in a PR and we discuss the details there. I think packages/chord-name-parse or something similar it's a good package name (but I'm open to ideas)

If you're looking for inspiration, @mathaou pointed recently to https://bspaans.github.io/python-mingus/ that has a nice parser. There's also a npm package called chord-magic that, if I remember well, it has another.

@lukephills
Copy link

lukephills commented May 5, 2020

I agree on @honkskillet 's points on fixing the naming inconsistencies. Just a thought on that though. What about exposing a method to allow people to set their own chord data object?

@jcleefw
Copy link

jcleefw commented Sep 26, 2020

Hi, I just came across this issue and found this topic of discussion. I read through the conversation and it sounded like some solutions are suggested. Was there work being done following the discussion?

I think @lukephills suggestion might not be a bad idea at all, almost like an extension of the subset.

Great lib by the way

@felixroos
Copy link
Contributor

Hey there, I was having similar problems with some chords. I wrote a little test script that exposes broken chord symbols:

import { ChordType } from '@tonaljs/tonal';

// get all aliases
const aliases = ChordType.all().reduce((all: string[], { aliases }) => all.concat(aliases), [])
// check aliases without octaves
const broken = aliases.filter(alias => {
  const tokens = Chord.tokenize('C' + alias);
  return tokens[0] !== 'C' || tokens[1] !== alias;
});

result:

[
  "67",
  "-#5",
  "-",
  "-Δ7",
  "-^7",
  "-7",
  "-6",
  "-7b5",
  "2",
  "69",
  "69#11",
  "-^9",
  "-9",
  "-69",
  "-13",
  "-11",
  "b9sus"
];
  • all those chord symbols will not work as expected
  • except b9sus, all of them start with a number or a "-" sign
  • some of those are my fault
  • b9sus is broken because it parses to ["Cb", "9sus"]

A quick and dirty fix would be to add all those to NUM_TYPES in chord/index.ts:

// const NUM_TYPES = /^(6|64|7|9|11|13)$/;
const NUM_TYPES = /^(6|64|7|9|11|13|76|-#5|67|-#5|-|-Δ7|-^7|-7|-6|-7b5|2|69|69#11|-^9|-9|-69|-13|-11)$/;

This fixes all expect "b9sus".

But I think the root of this problem is the aim to support chords with notes that have octaves.

I think the best solution is to suppose that chord never uses octaves in its names. So Chord.get('C2') will return an empty chord. But this is a breaking change. What do you think?

That might be a breaking change, but using octaves is already quite broken:

const brokenWithOctave = aliases.filter(alias => {
  const tokens = Chord.tokenize('C3' + alias);
  return tokens[0] !== 'C3' || tokens[1] !== alias;
});

result:

[
  "5",
  "7#5sus4",
  "7sus4",
  "7sus",
  "7no5",
  "7#5",
  "7+",
  "7aug",
  "7b13",
  "7",
  "6",
  "7add6",
  "67",
  "7add13",
  "7b6",
  "7b5",
  "7#11",
  "7#4",
  "6#11",
  "6b5",
  "7#11b13",
  "7b5b13",
  "4",
  "7#5#9",
  "7#9#5",
  "7alt",
  "7#9",
  "13#9",
  "7#9b13",
  "7#9#11",
  "7b5#9",
  "7#9b5",
  "13#9#11",
  "7#9#11b13",
  "9sus4",
  "9sus",
  "11",
  "13sus4",
  "13sus",
  "9no5",
  "13no5",
  "9b13",
  "9#5",
  "9+",
  "2",
  "9",
  "6/9",
  "69",
  "13",
  "9b5",
  "13b5",
  "9#5#11",
  "9#11",
  "9+4",
  "9#4",
  "69#11",
  "13#11",
  "13+4",
  "13#4",
  "9#11b13",
  "9b5b13",
  "7b9sus",
  "7b9sus4",
  "11b9",
  "7sus4b9b13",
  "7b9b13sus4",
  "7#5b9",
  "7b9#5",
  "7b9",
  "13b9",
  "7b9b13",
  "7#5b9#11",
  "7b9#11",
  "7b5b9",
  "7b9b5",
  "13b9#11",
  "7b9b13#11",
  "7b9#11b13",
  "7b5b9b13",
  "7b9#9"
]

As expected, no chord symbol with a number will work with octaves.

Removing octave support from Chord.get also would'nt hurt because getChord can be used.

Agree. I still think the best solution is to change Chord.get to receive (type, tonic) and remove any ambiguity

I think it would still be really useful to have chord tokenizer included with tonal.

An example of an ambiguous chord would be C#9

I think this is not a real problem, as this not only confuses a parses, but also people. I guess you would just have to write "CM7#9" if you want a c major 7 sharp 9...

@danigb
Copy link
Collaborator

danigb commented Nov 11, 2020

Thanks everyone for the ideas. It's clear the Chord.get is currently quite broken both for machines and humans.

To be fair, it's true that all music notation was invented long (or looooooong) before computers. For example: C#9. You can't know if it's C# 9 or C #9 ... the only way to discover is by knowing the context, something beyond the scope of this library.

Going back to specifics, there are two proposals:

1. Remove octave from name: I think this is a must to resolve the issue.

I'm 100% sure about this. I'll create PR.

2. Remove the ability to use more than one accidental to the root.

I'm not sure about that. It doesn't solve the issue (see C#9) but probably makes things more predictable. Thoughts?

There's one thing that bothers me: if we don't specify the octave, there are two options with the chord notes:

  • Use pitch classes. I don't like it very much: C9 would be C, E, G, D... and not sure if that's correct (and for sure, it's an information loss)
  • Use a predefined octave. For example, use always 4 oct as base. C9 notes would be C4, E4, G4, D5 (I like that it expresses better the chord, but is very arbitrary.

Removing octave support from Chord.get also would'nt hurt because getChord can be used.

Yes, that convinced me

I think it would still be really useful to have chord tokenizer included with tonal.

Agree

Thouhgts?

What do you think?

@felixroos
Copy link
Contributor

Hey, that was a quick answer :) I am happy that you are sure about removing octave support.

Use pitch classes. I don't like it very much: C9 would be C, E, G, D... and not sure if that's correct

Currently, this is already the case:

Chord.get('C9').notes // [ 'C', 'E', 'G', 'Bb', 'D' ]
Chord.get('C9').intervals // [ '1P', '3M', '5P', '7m', '9M' ]

(and for sure, it's an information loss)

I guess you mean that the interval structure is lost when just using pitch classes? I think that's not a big deal as you could use the intervals prop, or if you want octaves, just use getChord.

In my opinion, having the pitch classes without octaves is still pretty useful, as the interval structure of a chord is never fixed, and you can use whatever octave you like, for any note of the chord, it would still be called the same name, being just a different voicing.

I am currently using tonal to find chord voicings, check out my posts about voicing dictionaries and voicing permutation. Maybe some of that could also be a tonal package?

@danigb
Copy link
Collaborator

danigb commented Nov 11, 2020

I guess you mean that the interval structure is lost when just using pitch classes? I think that's not a big deal as you could use the intervals prop, or if you want octaves, just use getChord.

Yes, your right! Pitch classes.

I am currently using tonal to find chord voicings, check out my posts about voicing dictionaries and voicing permutation. Maybe some of that could also be a tonal package?

I haven't read fully (yet) but they look awesome!! This is kind of things I wanted to do (and never did) when I've started tonal. Thanks for sharing!

UPDATE: Adding voicing dictionaries was always on my mind. So feel free to open an issue to discuss and/or a PR to propse 👍

@abulka
Copy link

abulka commented Jul 1, 2022

I'm getting errors scanning some chord symbols e.g. Tonal.Chord.get('C69#11').

@felixroos Re your idea

A quick and dirty fix would be to add all those to NUM_TYPES in chord/index.ts:

Do you think it Is somehow possible to monkey patch Tonal so that I don't need to build it with this change - would rather use the normal version available via npm, you see.

I suppose another fix is to pre-process chord symbols to identify the tonic and add a space after the tonic letter because e.g. Tonal.Chord.get('C 69#11') works ok.

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

No branches or pull requests

6 participants