Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by address@hidden)

[Paul Morris]

Hi Carl,

On 4/2/19 12:14 AM, address@hidden 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