Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by chazwins6@gmail.com)

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

Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by chazwins6@gmail.com)

Carl Sorensen
On 2018/11/10 05:44:23, pwm wrote:

> https://codereview.appspot.com/337870043/diff/40001/Documentation/notation/chords.itely#newcode679
> Documentation/notation/chords.itely:679: represent the structure of
the chord.
> When entered in node mode,
> typo: "note mode"
Done.


https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-name-exceptions.ly#newcode4
> input/regression/chord-name-exceptions.ly:4: texidoc = "The property
> @code{chordNameExceptions} can used
> 'can be used' (This was carried over, but might as well fix while
we're here.)

Done



https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-name-exceptions.ly#newcode29
> input/regression/chord-name-exceptions.ly:29: chExceptions = #(append
> (chordmode->exception-entry chordVar markupVar) chExceptions)
> Hmm, chExceptions is used in its own definition here?  Does that work?
  This is
> not making sense to me.

chExceptions is previously defined to be a couple of new chords
prepended to ignatzekExceptions, so it can be used this way.





https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-lowercase-root.ly#newcode14
> input/regression/chord-semantics-lowercase-root.ly:14:
> Whitespace on unnecessary blank line.

Done


https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-name-exceptions.ly#newcode5
> input/regression/chord-semantics-name-exceptions.ly:5: texidoc = "The
property
> @code{chordSemanticsNameExceptions} can used
> can be used

Done


https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-power-chord.ly#newcode12
> input/regression/chord-semantics-power-chord.ly:12:
> Whitespace on unneeded blank line.


Done


https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode23
> scm/chord-entry.scm:23:
> Unnecessary new line.

Done

https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode75
> scm/chord-entry.scm:75: chord-semantics))
> It's hard to read this code because of the way it's formatted.  Would
be better
> with more line-breaks to keep the lines from being too long and the
indentation
> from going so far to the right and wrapping around to the next line
(in narrow
> views like this code review one).  Similar comment for other places in
this file
> like this.

Done



https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode238
> scm/chord-entry.scm:238: 'chord-semantics chord-semantics))
> Hmm, so there's a single 'chord-semantics property under a
'ChordSemanticsEvent
> ?  Seems like we could avoid that extra step of nesting and just have
the
> subproperties of 'chord-semantics under 'ChordSemanticsEvent ?  And
that would
> be more like NoteEvent and other events.  Or maybe I'm missing
something?


We could do that, but it would pollute the global namespace with all the
properties
that are part of 'chord-semantics and are only used for chord semantics.
  By putting them
in a single property, we avoid polluting the namespace.  Similar to the
way we do fret-diagram-details.


https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode243
> scm/chord-entry.scm:243: (ly:pitch-octave (entry-pitch
original-inv-pitch))))
> Would be more clear and consistent if original-inv-pitch were renamed
to
> original-inv-entry or similar.

Why?  Not to be argumentative, but I wonder what about this name is more
clear and consistent, in your opinion?



https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode267
> scm/chord-entry.scm:267: (append (if bass (write-me "base3: " bass)
(list
> (make-note-ev bass 'bass #t)) '())
> This write-me looks like a debugging statement that was left in?


Done


https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode272
> scm/chord-entry.scm:272: (bass (make-elements (cons (make-note-ev bass
'bass
> (cons #t #t))
> Hmm, this (cons #t #t) looks like it could be related to one of the
regression
> test failures that James posted about?

Will look more later.



https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode306
> scm/chord-entry.scm:306: (assoc-ref semantics-list key))
> This is defined twice, see line 292 above.

Done



https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode399
> scm/chord-entry.scm:399: ((= alteration FLAT) (set! quality 'dim)))))
> Instead of all of these (set! quality ...) why not just let the result
of this
> cond be assigned to 'quality' and add an 'else' 'maj at the end?
Basically:
> (quality (cond ((= alteration SHARP) 'aug) ...))

I think it's because for some intervals, you set a zero alteration as
perfect, while
for others, the zero alteration is 'maj.  So it doesn't drop easily into
an else, I think.



https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode411
> scm/chord-entry.scm:411: (if (= step-number 7) (set! quality 'min))
> Defining quality using cond or case would be a more idiomatic
Scheme-ish way to
> do it, avoiding the mutation of set!.

I totally agree with you; I'll try to work this out for a future patch.
For now, I want
to just get it into review.



https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode414
> scm/chord-entry.scm:414: (cons (list (cons 'step-number step-number)
(cons
> 'step-quality quality)) chord-step-list)
> Why not use make-chord-step here to simplify this?  Also, I'd wrap
this to more
> than one line, to break it up and make it easier to read and
understand.

Done



https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode425
> scm/chord-entry.scm:425: (make-chord-entry (ly:make-pitch 0 (- n 1)
(nca n))
> chord-step)))
> Here's a way to simplify this:

>   (map (lambda (chord-step)
>          (let* ((sn (assoc-ref chord-step 'step-number))
>                 (octave (if (>= sn 8) 1 0))
>                 (note (- sn (if (>= sn 8) 8 1)))
>                 (alter (if (= sn 7) FLAT 0))
>                 (pitch (ly:make-pitch octave note alter)))
>            (make-chord-entry pitch chord-step)))


Done


https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode44
> scm/chord-ignatzek-names.scm:44: "Does PS have the X step? Return that
step if
> it does."
> Usually Scheme doc strings come after the 'define' like the one in
double quotes
> here.  I think it would be better to do them that way, rather than
with ';;'
> above the 'define'.  That will allow consolidation of some of these
that have
> both here.  Also, ps is short for pitches, so it would be clearer to
rename with
> ces, es, or ch-es (or something) for chord entries here.


https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode62
> scm/chord-ignatzek-names.scm:62: "Copy PS, but replace the step of P
in PS."
> ps -> es, ces, ch-es or something here as well. Probably p -> e too.
And
> elsewhere as needed.

Done



https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode317
> scm/chord-ignatzek-names.scm:317: lowercase-root?))))))
> Indentation seems off here, compared to before this patch.

Done



https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode362
> scm/chord-ignatzek-names.scm:362: empty-markup))
> These make-removals-markup and make-additions-markup functions are
very similar.
>   If you are feeling motivated, it would be good to refactor to remove
the
> duplication.

Save for next review



https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode379
> scm/chord-ignatzek-names.scm:379:
> Whitespace to remove.


https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode395
> scm/chord-ignatzek-names.scm:395: (append
> Trailing whitespace nit.

Should be cleared up.



https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode405
> scm/chord-ignatzek-names.scm:405: (ly:context-property context
> 'chordPrefixSpacer))
> Formatting, line breaking, indentation causing this code to go too far
to the
> right.

Done


https://codereview.appspot.com/337870043/diff/40001/scm/chord-name.scm#newcode177
> scm/chord-name.scm:177: (filter not-root-entry? semantics-list))
> Could use a blank line between these two define-publics.


Done


https://codereview.appspot.com/337870043/diff/40001/scm/chord-name.scm#newcode181
> scm/chord-name.scm:181: ;; chordmode->exception-entry
> This comment doesn't add much.

Done



https://codereview.appspot.com/337870043/diff/40001/scm/chord-name.scm#newcode185
> scm/chord-name.scm:185: "
> Closing " usually doesn't get its own line.
Done


https://codereview.appspot.com/337870043/diff/40001/scm/chord-name.scm#newcode187
> scm/chord-name.scm:187: (define (get-semantics-event music)
> Optional, but it's probably clearer to move this define up a level and
not nest
> it under the other defines so deeply.


Done



https://codereview.appspot.com/337870043/diff/40001/scm/chord-name.scm#newcode204
> scm/chord-name.scm:204: return))
> The 'return' is not adding much here.  I'd suggest dropping it and
just having
> the result of the cond be the return value for the function, without
the (set!
> return ...) expressions, and with an else '() fallback.

Done



https://codereview.appspot.com/337870043/

_______________________________________________
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 GSoC code: Chord Semantics (issue 337870043 by chazwins6@gmail.com)

Paul Morris
Hi Carl,

On 4/2/19 12:14 AM, [hidden email] wrote:

> https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-name-exceptions.ly#newcode29 
>
>> input/regression/chord-name-exceptions.ly:29: chExceptions = #(append
>> (chordmode->exception-entry chordVar markupVar) chExceptions)
>> Hmm, chExceptions is used in its own definition here?  Does that
>> work? This is
>> not making sense to me.
>
> chExceptions is previously defined to be a couple of new chords
> prepended to ignatzekExceptions, so it can be used this way.

Ah, right, okay.


> https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode238 
>
>> scm/chord-entry.scm:238: 'chord-semantics chord-semantics))
>> Hmm, so there's a single 'chord-semantics property under a
>> 'ChordSemanticsEvent
>> ?  Seems like we could avoid that extra step of nesting and just have
>> the
>> subproperties of 'chord-semantics under 'ChordSemanticsEvent ?  And
>> that would
>> be more like NoteEvent and other events. Or maybe I'm missing something?
>
> We could do that, but it would pollute the global namespace with all the
> properties
> that are part of 'chord-semantics and are only used for chord semantics.
>  By putting them
> in a single property, we avoid polluting the namespace.  Similar to the
> way we do fret-diagram-details.

Okay, I see.


> https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode243 
>
>> scm/chord-entry.scm:243: (ly:pitch-octave (entry-pitch
>> original-inv-pitch))))
>> Would be more clear and consistent if original-inv-pitch were renamed to
>> original-inv-entry or similar.
>
> Why?  Not to be argumentative, but I wonder what about this name is more
> clear and consistent, in your opinion?

Should have said more at the time.  Here's what I think I was thinking.

Having 'pitch' in the name conveys that it is a LilyPond 'pitch' data
structure, but that isn't the case.  Here you have to call `entry-pitch`
on it to get a pitch out of it to pass to `ly:pitch-octave`.

`entry-pitch` works on `chord-entry` type things:

;; Return pitch of a chord-entry
(define (entry-pitch chord-entry)
   (car chord-entry))

So it seems to be more like a type of 'entry' structure, and avoiding
using 'pitch' in the name (like with original-inv-entry or similar)
makes it easier to understand what it is and what you can and can't do
with it.


> https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode399 
>
>> scm/chord-entry.scm:399: ((= alteration FLAT) (set! quality 'dim)))))
>> Instead of all of these (set! quality ...) why not just let the
>> result of this
>> cond be assigned to 'quality' and add an 'else' 'maj at the end?
>> Basically:
>> (quality (cond ((= alteration SHARP) 'aug) ...))
>
> I think it's because for some intervals, you set a zero alteration as
> perfect, while
> for others, the zero alteration is 'maj.  So it doesn't drop easily into
> an else, I think.

Ah, right, so we still need to take step-number into account. But
there's still refactoring that can be done along the lines I suggested,
as there are basically two cases, the major/minor and the perfect. 
Here's an untested refactor:

  (define (get-quality step-number alteration)
    (cond
     ;; major/minor
     ;; note from Paul: I added 7 and 14 which weren't in the original
     ((member step-number '(2 9 3 10 6 13 7 14))
      (case alteration
       ((SHARP) 'aug)
       ((0) 'maj)
       ((FLAT) 'min)
       ((DOUBLE-FLAT) 'dim)))
     ;; perfect
     ;; TODO: will 11 have the same qualities as 4?
     ((member step-number '(1 8 4 11 5 12))
      (case alteration
       ((SHARP) 'aug)
       ((0) 'perfect)
       ((FLAT) 'dim)))
     ;; default fallback
     (else 'maj)))

  (define (make-chord-entry-from-pitch pitch)
    (let* ((step-number (pitch-step pitch))
           (alteration (ly:pitch-alteration pitch))
           (quality (get-quality step-number alteration)))
      (make-chord-entry pitch (make-chord-step step-number quality))))


> https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode411 
>
>> scm/chord-entry.scm:411: (if (= step-number 7) (set! quality 'min))
>> Defining quality using cond or case would be a more idiomatic
>> Scheme-ish way to
>> do it, avoiding the mutation of set!.
>
> I totally agree with you; I'll try to work this out for a future patch.
> For now, I want
> to just get it into review.

Okay, makes sense.  Thanks for working on this.

-Paul



_______________________________________________
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 GSoC code: Chord Semantics (issue 337870043 by chazwins6@gmail.com)

Carl Sorensen
In reply to this post by Carl Sorensen


> Thanks for figuring this out.  I'm now working on make check, and will
post a
> new patch shortly (I hope).

The new patch is up at https://codereview.appspot.com/568650043


https://codereview.appspot.com/337870043/

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