Charles Winston's chord-semantics GSOC work

Respond to Paul Morris comments on issue 337870043

[Valentin Villenave, 2019-04-02 21:20:50]

Hi Carl,
I appreciate you taking the time to rework this patch, does it mean you’re
intending to shepherd Charles’ work until it gets merged?
In addition to Paul’s comments which you’ve nicely addressed, I had a few
additional ones below, on other aspects of Charles’ approach (and taking into
account that I’ve been trying to streamline some chord-related parts of
LilyPond’s codebase in the meantime).
BTW, is the end goal here to actually replace Lily’s default chord entry mode at
some point? As you may have noticed, I’ve dropped some chord modes that hadn’t
been working for the past 15 years anyway (namely Banter, and jazzExceptions as
well), so still referring to Ignatzek exceptions as such whilst there are none
other available has become sort of moot. Now that we’d be having an additional
`semantics’ feature, it *could* sort of make sense to have two different systems
in place (though not in the way chord names used to be implemented). At any
rate, I’ve been trying to hunt down code duplication and clunky mechanisms based
on hardcoded stuff (e.g. "Italian" and "German" chord names and note->string
functions); I’d hope that this patch would allow further progress in this regard
but that doesn’t appear to be the case.

https://codereview.appspot.com/568650043/diff/572560043/Documentation/notatio...
File Documentation/notation/chords.itely (right):

https://codereview.appspot.com/568650043/diff/572560043/Documentation/notatio...
Documentation/notation/chords.itely:472: c:m7^1 ees
I’d put the simple chord in front of the more unexpected use case.
Also, why not use c:maj7^1 and e:m chords?

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chor...
File input/regression/chord-name-exceptions.ly (right):

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chor...
input/regression/chord-name-exceptions.ly:1: \version "2.16.0"
Why not update the regtest version number? OTOH, it doesn’t actually introduces
a new syntax.

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chor...
File input/regression/chord-semantics-basic.ly (right):

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chor...
input/regression/chord-semantics-basic.ly:10: 
What’s this line standing for?

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chor...
File input/regression/chord-semantics-sus.ly (right):

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chor...
input/regression/chord-semantics-sus.ly:11: }
I feel like these are way too many regtests.  Sure, having a nicely ordered list
of all features looks nice, but 1/ we might be adding quite a few extra seconds
to `make check’ here 2/ regtests are not the place where to be listing or
documenting features and 3/ all of these could as well be included in a single
regtest, duly commented and explained: if anything ever breaks in the future,
we’ll catch it just as well.

https://codereview.appspot.com/568650043/diff/572560043/lily/chord-name-engra...
File lily/chord-name-engraver.cc (right):

https://codereview.appspot.com/568650043/diff/572560043/lily/chord-name-engra...
lily/chord-name-engraver.cc:99: SCM name_proc = get_property
("chordSemanticsNameFunction");
Ugh. Is there really no way of merging this with chordNameFunction (possibly
with  additionaloptargs)? I understand this adds many additional (and certainly
useful) features, but this looks like potential for duplicate/semi-incompatible
subroutines down the line.

Fortunately, both approaches use chordRootNamer and therefore will be able to
take advantage from the localized note names that are now available.  But still,
I wish we could factorize things even further.

https://codereview.appspot.com/568650043/diff/572560043/scm/chord-ignatzek-na...
File scm/chord-ignatzek-names.scm (right):

https://codereview.appspot.com/568650043/diff/572560043/scm/chord-ignatzek-na...
scm/chord-ignatzek-names.scm:342: (define (make-root-markup root
lowercase-root?)
I’m not sure "make-*-markup is the most unambiguous name for that subfunction.

[c_sorensen, 2019-04-03 00:36:47]

On 4/2/19, 3:20 PM, "v.villenave@gmail.com" <v.villenave@gmail.com> wrote:

    Hi Carl,
    I appreciate you taking the time to rework this patch, does it mean
    you’re intending to shepherd Charles’ work until it gets merged?

Yes.

    In addition to Paul’s comments which you’ve nicely addressed, I had a
    few additional ones below, on other aspects of Charles’ approach (and
    taking into account that I’ve been trying to streamline some
    chord-related parts of LilyPond’s codebase in the meantime).
    BTW, is the end goal here to actually replace Lily’s default chord entry
    mode at some point? 

Replacing chord entry mode is not part of the goal for this project.  The goal
is simply to capture the actual semantics entered by the user using the standard
LilyPond chord entry mode which is not based on the chord name, but based on the
chord steps and their qualities present in the chord.

Once you have the chord steps and their qualities, you should be able to derive
any naming system, as far as I can see. 

    As you may have noticed, I’ve dropped some chord
    modes that hadn’t been working for the past 15 years anyway (namely
    Banter, and jazzExceptions as well), so still referring to Ignatzek
    exceptions as such whilst there are none other available has become sort
    of moot. Now that we’d be having an additional `semantics’ feature, it
    *could* sort of make sense to have two different systems in place
    (though not in the way chord names used to be implemented). At any rate,
    I’ve been trying to hunt down code duplication and clunky mechanisms
    based on hardcoded stuff (e.g. "Italian" and "German" chord names and
    note->string functions); I’d hope that this patch would allow further
    progress in this regard but that doesn’t appear to be the case.

This patch creates a chord semantics structure and creates one namer based on
that structure.  Other namers could be added.

This patch does not change any of the note-to-chord-names functions.  It was
intended to not break any existing functionality.
But the chord-semantics namer should continue to work properly, even if the
note-to-chord-name functions are eliminated or changed.
    
I will respond to your individual comments below when I get a chance to rework
the patch, which may not be until the weekend.

Thanks,

Carl
 

[Carl, 2019-04-06 21:54:33]

Respond to Valentin and Paul

[Carl, 2019-04-06 22:06:22]

Thanks for the feedback!  New patch set uploaded.

https://codereview.appspot.com/568650043/diff/572560043/Documentation/notatio...
File Documentation/notation/chords.itely (right):

https://codereview.appspot.com/568650043/diff/572560043/Documentation/notatio...
Documentation/notation/chords.itely:472: c:m7^1 ees
On 2019/04/02 21:20:49, Valentin Villenave wrote:
> I’d put the simple chord in front of the more unexpected use case.
> Also, why not use c:maj7^1 and e:m chords?

Done.

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chor...
File input/regression/chord-name-exceptions.ly (right):

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chor...
input/regression/chord-name-exceptions.ly:1: \version "2.16.0"
On 2019/04/02 21:20:49, Valentin Villenave wrote:
> Why not update the regtest version number? OTOH, it doesn’t actually
introduces
> a new syntax.

Done.

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chor...
File input/regression/chord-semantics-sus.ly (right):

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chor...
input/regression/chord-semantics-sus.ly:11: }
On 2019/04/02 21:20:49, Valentin Villenave wrote:
> I feel like these are way too many regtests.  Sure, having a nicely ordered
list
> of all features looks nice, but 1/ we might be adding quite a few extra
seconds
> to `make check’ here 2/ regtests are not the place where to be listing or
> documenting features and 3/ all of these could as well be included in a single
> regtest, duly commented and explained: if anything ever breaks in the future,
> we’ll catch it just as well.

Upon review, I agree with you.  This is really a set of regtests for the
chord-semantics chord namer.  I've combined them into a single regtest.

https://codereview.appspot.com/568650043/diff/572560043/lily/chord-name-engra...
File lily/chord-name-engraver.cc (right):

https://codereview.appspot.com/568650043/diff/572560043/lily/chord-name-engra...
lily/chord-name-engraver.cc:99: SCM name_proc = get_property
("chordSemanticsNameFunction");
On 2019/04/02 21:20:49, Valentin Villenave wrote:
> Ugh. Is there really no way of merging this with chordNameFunction (possibly
> with  additionaloptargs)? I understand this adds many additional (and
certainly
> useful) features, but this looks like potential for
duplicate/semi-incompatible
> subroutines down the line.
> 
> Fortunately, both approaches use chordRootNamer and therefore will be able to
> take advantage from the localized note names that are now available.  But
still,
> I wish we could factorize things even further.

With this patch, there are two fundamental modes of getting the semantics
necessary to define a chord name.  The newly-added one is to use the semantics
entered by the user (chordSemanticsNameFunction); the previous one was to  parse
the pitches in the chord and try to infer the semantics.  (There is also the
pattern-matching provided by the exceptions that allows overriding the automatic
calculation of chord names, replacing it with a lookup function.) 

Since the difference between the two is how we get the semantics, I could see
that we replace the two fundamental chord name functions with a single function
having an optional argument which is a semantics entry.  If we want to use the
semantics entry present in the chord, we don't pass the optional argument.  If
we want to use the pitch parser chord namer, we could create a new function
whose job is to parse a set of pitches into a semantics entry.  The resulting
semantics entry could then be passed to the semantics-based namer. 

Or alternatively, we could always call the semantic namer, and have the semantic
namer call the parse-semantics function if there is no semantics entry in the
chord.  I believe this could be a way to achieve your goal.

However, this will require more refactoring.  I don't believe we should hold off
on this patch until we can get that part of it done better.  This patch has
lanquished long enough.  IMO we should just push it as-is and get it in the code
base.

https://codereview.appspot.com/568650043/diff/572560043/scm/chord-ignatzek-na...
File scm/chord-ignatzek-names.scm (right):

https://codereview.appspot.com/568650043/diff/572560043/scm/chord-ignatzek-na...
scm/chord-ignatzek-names.scm:342: (define (make-root-markup root
lowercase-root?)
On 2019/04/02 21:20:49, Valentin Villenave wrote:
> I’m not sure "make-*-markup is the most unambiguous name for that subfunction.

Good catch.

I've changed make-*-markup to make-*-display for all of the functions that are
local to this file.  Of course, the lillypond-defined make-*-markup functions
are not changed.

[Valentin Villenave, 2019-04-08 18:17:54]

On 2019/04/06 22:06:22, Carl wrote:
> However, this will require more refactoring.  I don't believe we should hold
off
> on this patch until we can get that part of it done better.  This patch has
> lanquished long enough.  IMO we should just push it as-is and get it in the
code
> base.

Agreed. LGTM, and thanks for your work on that!

Cheers,
V.

[lilypond-pkx, 2019-04-08 18:47:13]

Are we sure all the reg tests are OK (see tracker for download link)?

For example

regression/chord-name-major7.ly

This looks completely broken with this patch

[thomasmorley651, 2019-04-08 19:59:57]

On 2019/04/08 18:47:13, lilypond-pkx wrote:
> Are we sure all the reg tests are OK (see tracker for download link)?
> 
> For example
> 
> regression/chord-name-major7.ly
> 
> This looks completely broken with this patch

I had a look at the regtest-results, too. Though, I didn't check for changed
code in the regtests.
That said, here my impressions:


input/regression/chord-voicings.ly
#11 always becomes 11
On purpose? Not sure.

input/regression/spacing-non-adjacent-columns3.ly
F9add13add15 becomes F13add15!?
On purpose? Not sure.

input/regression/one-staff.ly
B7b5 should be printed as before, imho.
Thus, broken.

input/regression/chord-name-override-text.ly
Shouldn't there be a "foo"?
Likely broken

input/regression/chord-name-major7.ly
Broken.

input/regression/chord-names-languages.ly
Broken, but germanChords are often insufficient anyway...

input/regression/predefined-fretboards.ly
Absolutely broken.


Carl, many thanks working on it, but I'm sorry, I think there are some
show-stopper for now.

[thomasmorley651, 2019-04-08 19:59:57]

On 2019/04/08 18:47:13, lilypond-pkx wrote:
> Are we sure all the reg tests are OK (see tracker for download link)?
> 
> For example
> 
> regression/chord-name-major7.ly
> 
> This looks completely broken with this patch

I had a look at the regtest-results, too. Though, I didn't check for changed
code in the regtests.
That said, here my impressions:


input/regression/chord-voicings.ly
#11 always becomes 11
On purpose? Not sure.

input/regression/spacing-non-adjacent-columns3.ly
F9add13add15 becomes F13add15!?
On purpose? Not sure.

input/regression/one-staff.ly
B7b5 should be printed as before, imho.
Thus, broken.

input/regression/chord-name-override-text.ly
Shouldn't there be a "foo"?
Likely broken

input/regression/chord-name-major7.ly
Broken.

input/regression/chord-names-languages.ly
Broken, but germanChords are often insufficient anyway...

input/regression/predefined-fretboards.ly
Absolutely broken.


Carl, many thanks working on it, but I'm sorry, I think there are some
show-stopper for now.

[c_sorensen, 2019-04-09 00:29:34]

On 4/8/19, 1:59 PM, "thomasmorley65@gmail.com" <thomasmorley65@gmail.com>
wrote:

    On 2019/04/08 18:47:13, lilypond-pkx wrote:
    > Are we sure all the reg tests are OK (see tracker for download link)?
    
    > For example
    
    > regression/chord-name-major7.ly
    
    > This looks completely broken with this patch

Interesting question.  The semantics that Charles coded says that c:maj7 is the
way to indicate the C major 7 semantics.  This regtest had c:7+, which gives the
same pitches as c:maj7, but has a different semantic spelling.  In my opinion,
c:7+ should probably be named C#7, not C triangle.  Of course, the code is
broken because it loses the alteration on the extension (but not the addition).
    
    
    input/regression/chord-voicings.ly
    #11 always becomes 11
    On purpose? Not sure.

Same problem as before: the alteration on the extension is not typeset.  Bug
that needs fixing.

    
    input/regression/spacing-non-adjacent-columns3.ly
    F9add13add15 becomes F13add15!?
    On purpose? Not sure.

Definitely not on purpose.  Need to investigate.
    
    input/regression/one-staff.ly
    B7b5 should be printed as before, imho.
    Thus, broken.

The current semantics is entered as b:m7.5-, which is Bm7b5.  The half-dim
symbol was part of the ignatzek exceptions.  Probably need to add it to
semantic-exceptions.
    
    input/regression/chord-name-override-text.ly
    Shouldn't there be a "foo"?
    Likely broken

Yes -- this needs investigation.

    
    input/regression/chord-names-languages.ly
    Broken, but germanChords are often insufficient anyway...
    
    input/regression/predefined-fretboards.ly
    Absolutely broken.

I agree.  This is probably due to the use of chord shapes --- need to figure out
how best to handle it (probably drop semantics from chord shapes).
    
    
    Carl, many thanks working on it, but I'm sorry, I think there are some
    show-stopper for now.
    
You're absolutely right.  I've made some progress, but it will probably be next
weekend before I can finish it.

Thanks,

Carl

    
    
    https://codereview.appspot.com/568650043/