Voicing Package #224

felixroos · 2020-11-12T20:31:35Z

As already proposed in #223, this PR contains a first version of the Voicing package.
For now, this contains:

Voicing.get
Voicing.search
Voicing.lookup
Voicing.sequence
VoiceLeading.topNoteDiff
VoicingDictionary.lefthand (classic jazz left hand voicings)
VoicingDictionary.triad (all triad inversions)
VoicingDictionary.all (above 2 combined)
enharmonicEquivalent (possibly needs to be moved to Note)

I also wrote basic test for all functions.

Open points:

I followed the steps in CONTRIBUTING.md, but running yarn test:ci generates a lot of changes that look like a different code formatting...
enharmonicEquivalent is currently a little lost in here. I think it could be a good fit for Note. What do you think?
VoicingDictionary needs more sets of voicings (non blocking)
VoiceLeading needs more methods (non blocking)

I tried to adapt to the existing file structure / code style, but maybe there are things that could be optimized.
What do you think? I will now wait for your feedback, and maybe collect some voicings in the meantime 😊

This reverts commit a0d3325.

danigb · 2020-11-12T20:57:06Z

Thanks a lot for both for #223 and this ❤️ . I'll read and review in depth this weekend. Meanwhile, in a quick pass I've seen:

All modules should include a README with a description and API documentation.
You should include the new module in the Tonal package
Why are some test commented?

It would be nice if you can add missing features while waiting 😇

Thanks again!

danigb · 2020-11-12T21:34:11Z

By the way, I agree that enharmonicEquivalent should be inside Note module (or at least, not here), so maybe we can start with a smaller PR with that change.

Also, I have to say that I was surprised that wasn't the actual behaviour of the Note.enharmonic function itself.

For example, you can do this:

Note.enharmonic('B#4') // => 'C5'

But unfortunately, not this:

Note.enharmonic('F2') // => 'F2'

So I guess if it is just better to modify Note.enharmonic to accept a second optional parameter.

Something like you did: Note.enharmonic('F2', 'E#') // => 'E#2'

Of course, it should still work with pitch classes: Note.enharmonic('F', 'E#') // => 'E#' (but it only works to verify that the second parameter is an enharmonic)

And I guess something like Note.enharmonic('F#3', 'Eb') should return an empty string.

What do you think? Makes sense? Is this enough for your needs?

danigb · 2020-11-12T21:51:57Z

Just to remember, as I said at #223:

Function names can be improved. For example, it's not easy to know what is the difference between searh and lookup
Probably it's a good idea to have three separate packages: voicing-dictionary, voice-leading and voicing (and three separated PRs)

felixroos · 2020-11-13T08:29:00Z

Ok, then the packages will contain the following:

Voicing (this PR)

get
search
lookup
sequence
names to be discussed

VoiceLeading (extra PR)

topNoteDiff
more to come

VoicingDictionary (extra PR)

lefthand / rootless
triad (basic triad inversions)
all / default => will be used by Voicing as default
https://www.learnjazzstandards.com/blog/learning-jazz/piano/encylcopedia-left-hand-jazz-piano-voicings/
https://www.thejazzpianosite.com/jazz-piano-lessons/jazz-chord-voicings/rootless-voicings/
https://www.thejazzpianosite.com/jazz-piano-lessons/jazz-chords/shell-chords/
many more possible

Note (extra PR)

enharmonic => add second param to implement enharmonicEquivalent

right? I could seperate the code sometime this weekend

felixroos · 2020-11-13T08:34:44Z

All modules should include a README with a description and API documentation.

yeah i forgot that

Why are some test commented?

those are possible future features, expressed as tests (the optional ones described in #223). That should not stay there..

You should include the new module in the Tonal package

ok

danigb · 2020-11-13T10:54:59Z

Great, it looks like we have a plan. Nice addition the links to the voicing dictionaries 👏 I can prepare the enharmonic function if you prefer 👍

felixroos · 2020-11-13T10:58:26Z

Great, it looks like we have a plan. Nice addition the links to the voicing dictionaries 👏 I can prepare the enharmonic function if you prefer 👍

Perfect! Those are the three tests that would need to pass for it to work:

test('Note.enharmonic with second param', () => {
  expect(Note.enharmonic('F2', 'E#')).toBe('E#2');
  expect(Note.enharmonic('B2', 'Cb')).toBe('Cb3');
  expect(Note.enharmonic('C2', 'B#')).toBe('B#1');
});

felixroos · 2020-11-13T19:24:48Z

Just pushed an update. I have now

split the code to the three packages voicing, voicing-dictionary and voice-leading.
added readme files with API doc to each package
added each package to tonal package

What's open

So far, I did not open separate pull requests for the other 2 packages. Is this really needed? I am not so used to the github PR workflow, and already committed.. Also, the voicing package depends on voicing-dictionary and voice-leading, so this is maybe better merged at once?
the enharmonic function is currently "polyfilled" in voicing/enharmonic.ts, and needs to be replaced when part of Note
I did not yet change the function naming
Still not enough content in voice-leading & voicing-dictionary packages
Anything else?
EDIT: also there is still an issue with code formatting, so I could not yet run yarn test:ci without generating a ton of changes
EDIT: also the test "@tonaljs/chord-type" > "names" currently fails when running test:ci

felixroos · 2020-11-13T19:47:54Z

packages/tonal/index.ts

 import VoiceLeading from '@tonaljs/voice-leading';
+import VoicingDictionary from '@tonaljs/voicing-dictionary';


hmm the pre commit hook messed something up here. I actually wanted to revert formatting everything with single quotes

Tonal enforces prettier to be used, but it seems that is not working with you? 🤔 You can npm run format to format the code (that's what the pre commit hook does). And it should result with double quotes. I don't know why is not the case...
EDIT: are you using a custom prettier config?

I assume it uses my local prettier config because the project does not contain a prettier config

npm run format also formats everything wrong (things that I did not even touch)

it might help adding a .prettierrc with your local setup

You're right! I thought it was one... let me add it 👍

I've added a prettier configuration to package.json file: 638b5ef

Can you try to add the line "prettier": {}, to your package.json to see if it solves the problem?

danigb · 2020-11-13T23:47:16Z

Hi @felixroos,

Just pushed an update. I have now (...)

🎉 Awesome...

So far, I did not open separate pull requests for the other 2 packages. Is this really needed?

Well, I'd really like it to be that way, because we could take care of little details of each.

I'd start with dictionary. For example, I'm not convinced about lefthand becase it's too attached to a piano keyboard. Then move into voice-leading (not sure yet what does exactly, and I'm missing the tests, for example), and then review voicing.

But I understand it's a lot of work, so I had an idea:

add some final fixes to this PR (see summary below)
mark modules as "private" until we polish them
iterate a bit over the modules creating new issues and/or PRs
when ready, publish

How does it sound?

the enharmonic function is currently "polyfilled" in voicing/enharmonic.ts, and needs to be replaced when part of Note

I've just merged a PR with the second Note.enharmonic function.

I did not yet change the function naming

Yes, this is something I definitely want to fix before publishing (but we can merge and discuss the names in an issue).

Still not enough content in voice-leading & voicing-dictionary packages

It's ok. But it will be great to add the source links into a "Sources" / "Resources" section at dictionary's README.

Anything else?

Well, in summary:

Fix formatting issue
Fix failing test
Mark packages as private (inside each package.json)
Add voice leading tests

What do you think?

EDIT: also there is still an issue with code formatting, so I could not yet run yarn test:ci without generating a ton of changes

Yes, this is a must before merge this PR

EDIT: also the test "@tonaljs/chord-type" > "names" currently fails when running test:ci

Weird, it works with for me... 🤔 You didn't change anything at chords dictionary, right? 😂

In any case, thank you very much! Let's fix those issues and merge. Good work!! 👏 👏

Upstream Changes

felixroos · 2020-11-14T12:51:45Z

Hey, this is slowly getting somewhere!

Well, in summary:

Fix formatting issue

Fix failing test

Mark packages as private (inside each package.json)

Add voice leading tests

What do you think?

That sounds like a plan.

Fix formatting issue

Your change (adding prettier: {} to package.json) did the trick! I now ran formatting and it just modified the files I touched. Done!

Fix failing test

This is the failed test:

Error: expect(received).toEqual(expected) // deep equality

- Expected  - 1
+ Received  + 1

  Array [
    "fifth",
    "suspended fourth",
    "suspended fourth seventh",
    "augmented",
-   "major seventh flat sixth",
+   "augmented seventh",
  ]

It looks unrelated to any changes i've made (only added packages + changed tonal/index.ts + tonal/package.json). The test would pass if I adjusted the above lines.

Mark packages as private

done

Add voice leading tests

So far there is one test for the only function. More to come

I've just merged a PR with the second Note.enharmonic function.

After integrating the upstream changes, the Note.enharmonic function is now available with two params! EDIT: It works! I just had to rebuild... Thank you

I'm not convinced about lefthand becase it's too attached to a piano keyboard

I understand that. I think it is kind of difficult to name voicings (also chords), as there is no "musicians committee for naming conventions" and there might be multiple ways of naming things.

Being just a monophonic trumpet player myself, my knowledge about voicings mainly comes from 2 years of piano lessons in my jazz conservatory (where I just learned about lefthand voicings). Luckily, I have an old friend who is a professional jazz piano player who could maybe help me out with that.

It might be debatable, but I think it is more important to "keep it real", using actual names that are known to musicians (as inconsistent as they might sometimes seem) than to use names that might be more logical, but not really used that way.

we can merge and discuss the names in an issue

Alright

Summary:

fix failing tests (could you look into that?)
- "@tonaljs/chord-type" > "names"
- ~~"@tonaljs/voicing" > "Cminor7 lefthand", related to Note.enharmonic~~
~~add voice leading tests~~ EDIT: done
discuss names in a separate issue
Something else?

+ move VoiceLeading test

felixroos · 2020-11-14T19:16:15Z

After thinking a little about possible future features, I had two ideas that could prevent possible breaking changes in the future:

1. VoicingDictionary should enable mapping multiple chord symbols to one set of intervals

Before:

export const lefthand: VoicingDictionary = {
  m7: ["3m 5P 7m 9M", "7m 9M 10m 12P"],
  /* more symbols */
}

after:

const lefthand = [
  [['m7','m9'], ["3m 5P 7m 9M", "7m 9M 10m 12P"]],
  /* more symbols */
];

This format allows mapping more than one symbol to a set of intervals. This resembles how many chord symbols are sometimes used synonymously. For example, in a jazz leadsheet, a "m7" is mostly played with a 9 (like above). If the sheet contains a "m9" instead, it is played exactly the same.

Additionally, this makes it possible to define the same symbol multiple times, which could be useful for some scenarios. VoicingDictionary.lookup should therefore filter all dictionary items by the given symbol and concat all sets that match.

2. Voicing.get could be more general

To allow adding other strategies to find voicings later (without needing to change Voicing.get / add a separate method).

Before:

function get(
  chord: string,
  range: string[] = defaultRange,
  dictionary = defaultDictionary,
  voiceLeading = defaultVoiceLeading,
  lastVoicing?: string[]
)

After:

function get(
  chord: string,
  voicingGenerator: VoicingGenerator,
  voiceLeading = defaultVoiceLeading,
  lastVoicing?: string[]
)

This method now has no hard wired connection to voicing dictionary. The voicingGenerator param expects a function that takes a chord and returns a set of intervals:

type VoicingGenerator = (chord: string) => IntervalSet;

As one way of generating voicings, we could create a generator with VoicingDictionary:

VoicingDictionary.generator(range, dictionary) => VoicingGenerator;

Example:

const lookup = VoicingDictionary.generator(['E3','D5'], lefthand);
lookup('C^7')
/*
  ["E3", "G3", "B3", "D4"],
  ["E4", "G4", "B4", "D5"],
  ["B3", "D4", "E4", "G4"],
*/

// in practice, we might directly pass the generator to Voicing.get:
const { topNoteDiff } = VoiceLeading;
const lastVoicing = ["C4", "E4", "G4", "B4"];
Voicing.get('F^7', lookup,  topNoteDiff, lastVoicing);
// ["C4", "E4", "F4", "A4"]

Currently, there is only one way of generating voicings (dictionary), but this call signature would allow adding others later (like voicing permutation).

Please tell me if this makes any sense? It is kind of abstract but I think it could be really powerful.

felixroos · 2020-11-15T11:59:28Z

I just made a little codesandbox to play around with (click "Open Sandbox" to edit)
UPDATE: just added piano sound + demo of Voicing.sequence

danigb · 2020-11-15T20:37:38Z

I just made a little codesandbox to play around with (click "Open Sandbox" to edit)

Wow! Nice! 😮

It might be debatable, but I think it is more important to "keep it real", using actual names that are known to musicians (as inconsistent as they might sometimes seem) than to use names that might be more logical, but not really used that way.

Very good point! 👍

After thinking a little about possible future features, I had two ideas that could prevent possible breaking changes in the future

Let's discuss in a separate issue

...

So, I think we are ready to merge (and continue the discussion...).

felixroos · 2020-11-15T21:18:05Z

Yiha, thanks for the merge! I created three issues for the open points. See you there

abulka · 2022-07-01T06:21:25Z

Even though it is merged, I cannot find any mention of this package in the Tonal README. Is the the Voicing package somehow available to be used? Any docs?

felixroos · 2022-07-01T06:58:26Z

Even though it is merged, I cannot find any mention of this package in the Tonal README. Is the the Voicing package somehow available to be used? Any docs?

Since the merge, it has never been release and it's still private. There are still some open issues:

These are now almost 2 years old and nothing has changed since then.. tbh I think I lost interest back then because the integration felt a little tedious.

I ended up publishing my own package with the same functionality: https://github.com/felixroos/chord-voicings

Still it would be nice to have this stuff integrated into tonal, so more people could use it.

You can play around with the chord voicings package here: https://strudel.tidalcycles.org?wVExAEFBUPQB

felixroos added 5 commits November 12, 2020 17:39

add voicing package

ab77769

simplify structure + fix tests

3a9314a

fix chord-type test

a0d3325

Revert "fix chord-type test"

4a367c2

This reverts commit a0d3325.

also export VoicingDictionary

44ff449

danigb mentioned this pull request Nov 12, 2020

Voicing Package #223

Closed

separate voicing-dictionary & voice-leading

f2042bb

use double quotes

5cd5562

felixroos commented Nov 13, 2020

View reviewed changes

Update README.md

2527129

danigb mentioned this pull request Nov 13, 2020

feat/enharmonic equivalent #225

Merged

felixroos and others added 4 commits November 14, 2020 13:05

mark packages as private

fdaf5db

Merge pull request #1 from tonaljs/master

0e76f37

Upstream Changes

Merge branch 'master' of https://github.com/felixroos/tonal

de566e0

now correctly code formatted

a7836ae

felixroos added 2 commits November 14, 2020 18:05

use Note.enharmonic

b5b1dd5

+ move VoiceLeading test

comment topNoteDiff example

833ee72

danigb merged commit 1deafd8 into tonaljs:master Nov 15, 2020

This was referenced Nov 15, 2020

VoicingDictionary should enable mapping multiple chord symbols to one set of intervals #227

Open

Voicing.get could be more general #228

Open

Voicing method names #229

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Voicing Package #224

Voicing Package #224

felixroos commented Nov 12, 2020

danigb commented Nov 12, 2020

danigb commented Nov 12, 2020

danigb commented Nov 12, 2020

felixroos commented Nov 13, 2020

felixroos commented Nov 13, 2020

danigb commented Nov 13, 2020

felixroos commented Nov 13, 2020

felixroos commented Nov 13, 2020 •

edited

Loading

felixroos Nov 13, 2020

danigb Nov 13, 2020 •

edited

Loading

felixroos Nov 13, 2020

felixroos Nov 13, 2020 •

edited

Loading

felixroos Nov 13, 2020

danigb Nov 13, 2020

danigb Nov 13, 2020

danigb commented Nov 13, 2020

felixroos commented Nov 14, 2020 •

edited

Loading

felixroos commented Nov 14, 2020 •

edited

Loading

felixroos commented Nov 15, 2020 •

edited

Loading

danigb commented Nov 15, 2020 •

edited

Loading

felixroos commented Nov 15, 2020

abulka commented Jul 1, 2022

felixroos commented Jul 1, 2022

		import VoiceLeading from '@tonaljs/voice-leading';
		import VoicingDictionary from '@tonaljs/voicing-dictionary';

Voicing Package #224

Voicing Package #224

Conversation

felixroos commented Nov 12, 2020

danigb commented Nov 12, 2020

danigb commented Nov 12, 2020

danigb commented Nov 12, 2020

felixroos commented Nov 13, 2020

felixroos commented Nov 13, 2020

danigb commented Nov 13, 2020

felixroos commented Nov 13, 2020

felixroos commented Nov 13, 2020 • edited Loading

felixroos Nov 13, 2020

Choose a reason for hiding this comment

danigb Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

felixroos Nov 13, 2020

Choose a reason for hiding this comment

felixroos Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

felixroos Nov 13, 2020

Choose a reason for hiding this comment

danigb Nov 13, 2020

Choose a reason for hiding this comment

danigb Nov 13, 2020

Choose a reason for hiding this comment

danigb commented Nov 13, 2020

felixroos commented Nov 14, 2020 • edited Loading

felixroos commented Nov 14, 2020 • edited Loading

1. VoicingDictionary should enable mapping multiple chord symbols to one set of intervals

2. Voicing.get could be more general

felixroos commented Nov 15, 2020 • edited Loading

danigb commented Nov 15, 2020 • edited Loading

felixroos commented Nov 15, 2020

abulka commented Jul 1, 2022

felixroos commented Jul 1, 2022

felixroos commented Nov 13, 2020 •

edited

Loading

danigb Nov 13, 2020 •

edited

Loading

felixroos Nov 13, 2020 •

edited

Loading

felixroos commented Nov 14, 2020 •

edited

Loading

felixroos commented Nov 14, 2020 •

edited

Loading

felixroos commented Nov 15, 2020 •

edited

Loading

danigb commented Nov 15, 2020 •

edited

Loading