Charles Winston's chord-semantics GSOC work (issue 568650043 by Carl.D.Sorensen@gmail.com)

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Charles Winston's chord-semantics GSOC work (issue 568650043 by Carl.D.Sorensen@gmail.com)

v.villenave
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/notation/chords.itely
File Documentation/notation/chords.itely (right):

https://codereview.appspot.com/568650043/diff/572560043/Documentation/notation/chords.itely#newcode472
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/chord-name-exceptions.ly
File input/regression/chord-name-exceptions.ly (right):

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-name-exceptions.ly#newcode1
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/chord-semantics-basic.ly
File input/regression/chord-semantics-basic.ly (right):

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

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

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-semantics-sus.ly#newcode11
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-engraver.cc
File lily/chord-name-engraver.cc (right):

https://codereview.appspot.com/568650043/diff/572560043/lily/chord-name-engraver.cc#newcode99
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-names.scm
File scm/chord-ignatzek-names.scm (right):

https://codereview.appspot.com/568650043/diff/572560043/scm/chord-ignatzek-names.scm#newcode342
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.

https://codereview.appspot.com/568650043/
_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Charles Winston's chord-semantics GSOC work (issue 568650043 by Carl.D.Sorensen@gmail.com)

Carl Sorensen-3


On 4/2/19, 3:20 PM, "[hidden email]" <[hidden email]> 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
 

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Charles Winston's chord-semantics GSOC work (issue 568650043 by Carl.D.Sorensen@gmail.com)

Carl Sorensen
In reply to this post by v.villenave
Thanks for the feedback!  New patch set uploaded.


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

https://codereview.appspot.com/568650043/diff/572560043/Documentation/notation/chords.itely#newcode472
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/chord-name-exceptions.ly
File input/regression/chord-name-exceptions.ly (right):

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-name-exceptions.ly#newcode1
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/chord-semantics-sus.ly
File input/regression/chord-semantics-sus.ly (right):

https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-semantics-sus.ly#newcode11
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-engraver.cc
File lily/chord-name-engraver.cc (right):

https://codereview.appspot.com/568650043/diff/572560043/lily/chord-name-engraver.cc#newcode99
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-names.scm
File scm/chord-ignatzek-names.scm (right):

https://codereview.appspot.com/568650043/diff/572560043/scm/chord-ignatzek-names.scm#newcode342
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.

https://codereview.appspot.com/568650043/
_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Charles Winston's chord-semantics GSOC work (issue 568650043 by Carl.D.Sorensen@gmail.com)

v.villenave
In reply to this post by v.villenave
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.

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

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Charles Winston's chord-semantics GSOC work (issue 568650043 by Carl.D.Sorensen@gmail.com)

pkxgnugitcl
In reply to this post by v.villenave
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

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

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Charles Winston's chord-semantics GSOC work (issue 568650043 by Carl.D.Sorensen@gmail.com)

Thomas Morley-2
In reply to this post by v.villenave
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.



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

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Charles Winston's chord-semantics GSOC work (issue 568650043 by Carl.D.Sorensen@gmail.com)

Thomas Morley-2
In reply to this post by v.villenave
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.



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

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Charles Winston's chord-semantics GSOC work (issue 568650043 by Carl.D.Sorensen@gmail.com)

Carl Sorensen-3


On 4/8/19, 1:59 PM, "[hidden email]" <[hidden email]> 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/
   

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel