-
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
Unexpected results from Chord.get() ? #155
Comments
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(). |
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. |
Hi @honkskillet Glad you find Tonal useful 👍 I think you're right in all the points. First, I'm aware of the inconsistencies of On the naming, you're also right. A PR will be awesome 🎉 |
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.) |
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? |
BTW, currently 'C##9' yields 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"] 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. |
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? |
Hi, thanks for all comments! Try to answer 👇
Agree. I still think the best solution is to change
I think that's an error. It should be:
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 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. |
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? |
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 |
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"
];
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.
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.
I think it would still be really useful to have chord tokenizer included with tonal.
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... |
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:
Yes, that convinced me
Agree Thouhgts? What do you think? |
Hey, that was a quick answer :) I am happy that you are sure about removing octave support.
Currently, this is already the case: Chord.get('C9').notes // [ 'C', 'E', 'G', 'Bb', 'D' ]
Chord.get('C9').intervals // [ '1P', '3M', '5P', '7m', '9M' ]
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? |
Yes, your right! Pitch classes.
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 👍 |
I'm getting errors scanning some chord symbols e.g. @felixroos Re your idea
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. |
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" ] }
The text was updated successfully, but these errors were encountered: