Hello, This fails a make check Processing `./fd/lily-2f531b63.ly' Parsing... Renaming input to: `/home/james/lilypond-git/input/regression/predefined-fretboards.ly' warning: cannot ...
6 years, 5 months ago
(2017-11-04 12:29:51 UTC)
#1
Hello,
This fails a make check
Processing `./fd/lily-2f531b63.ly'
Parsing...
Renaming input to:
`/home/james/lilypond-git/input/regression/predefined-fretboards.ly'
warning: cannot find property type-check for `chord-semantics' (music-type?).
perhaps a typing error?
Writing timing to fd/lily-2f531b63.profile...
programming error: Parsed object should be dead: #<Lily_lexer (#<module
2af6562478e0>) >
continuing, cross fingers
The following reg tests throw errors during make check:
/regression/fret-diagrams-string-thickness.ly
/regression/predefined-fretboards.ly
/regression/chord-changes.ly
/regression/identifier-following-chordmode.ly
/regression/fret-diagrams-string-frets.ly
/input/regression/chord-changes-alternative.ly
Charles, I just wanted to make sure you saw my last reply about your patch ...
6 years, 5 months ago
(2017-11-23 14:06:37 UTC)
#2
Charles,
I just wanted to make sure you saw my last reply about your patch failing the
regression tests. It would be shame to let all your hard work so far go to
waste.
Regards
james
On 2017/12/29 22:38:54, chazwins6 wrote: > Fixing errors with chord-semantics This should be working now. ...
6 years, 3 months ago
(2017-12-29 22:40:33 UTC)
#4
On 2017/12/29 22:38:54, chazwins6 wrote:
> Fixing errors with chord-semantics
This should be working now. Sorry for the delay, I was very busy with school!
Hello Charles, Thanks for your patch however, this fails make check. It fails on a ...
6 years, 3 months ago
(2017-12-30 18:10:53 UTC)
#5
Hello Charles,
Thanks for your patch however, this fails make check.
It fails on a number of different reg tests but all with the same basic error.
Example:
~~~
/home/james/lilypond-git/input/regression/chord-names-lower-case-minor.ly'
warning: cannot find property type-check for `chord-semantics' (music-type?).
perhaps a typing error?
Writing timing to 77/lily-34d19eca.profile...
programming error: Parsed object should be dead: #<Lily_lexer (#<module
2b102b5ce7c0>) >
continuing, cross fingers
~~~
and
~~~
/home/james/lilypond-git/input/regression/fret-diagrams-fret-label.ly'
warning: cannot find property type-check for `chord-semantics' (music-type?).
perhaps a typing error?
Writing timing to 97/lily-43513982.profile...
programming error: Parsed object should be dead: #<Lily_lexer (#<module
2b102b5c8ba0>) >
continuing, cross fingers
~~~
and
~~~
/home/james/lilypond-git/input/regression/chord-semantics-bass.ly'
warning: type check for `chord-step' failed; value `bass' must be of type `pair'
warning: cannot find property type-check for `chord-semantics' (music-type?).
perhaps a typing error?
Writing timing to d3/lily-efd6a06c.profile...
programming error: Parsed object should be dead: #<Lily_lexer (#<module
2b102b5cc860>) >
continuing, cross fingers
~~~
etc.
Hi Charles, Today I built and ran 'make check' with your patch applied to current ...
5 years, 5 months ago
(2018-11-10 19:44:47 UTC)
#10
Hi Charles, Today I built and ran 'make check' with your patch applied to
current master. I was able to get it to pass 'make check' by making the
following two changes.
1. In `chord-entry.scm` line 267, remove `(write-me "base3: " bass)`.
2. In that same file, line 100, remove the parens to change `(chord-semantics)`
to just `chord-semantics`.
The first change fixed this error (but note the type check warning):
input/regression/chord-names-languages.ly'
~~~
Parsing...
Renaming input to:
`/home/james/lilypond-git/input/regression/chord-names-languages.ly'
warning: type check for `bass' failed; value `(#t . #t)' must be of type
`boolean'
/home/james/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:267:59:
In procedure memoization in expression (if ba
ss (write-me "base3: " bass) ...):
/home/james/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:267:59:
In file "/home/james/lilypond-git/build/out/s
hare/lilypond/current/scm/chord-entry.scm", line 266: Missing or extra
expression in (if bass (write-me "base3: " bass) (list (make
-note-ev bass (quote bass) #t)) (quote ())).
~~~
And the second change fixed this error:
input/regression/chord-name-entry.ly
~~~
$ /home/dev/lilypond-git/build/out/bin/lilypond
input/regression/chord-name-entry.ly
GNU LilyPond 2.21.0
Processing `input/regression/chord-name-entry.ly'
Parsing.../home/dev/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:100:35:
In expression (chord-semantics):
/home/dev/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:100:35:
Wrong type to apply: ((modifier . #f) (root . #<Pitch c' >) (extension . 7)
(additions) (removals) (bass . #f))
~~~
So if you have a chance to upload a new patch set with those two changes, that
should get things moving forward with the code review process.
Cheers,
-Paul
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. > ...
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-na...
> 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-na...
> 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-se...
> input/regression/chord-semantics-lowercase-root.ly:14:
> Whitespace on unnecessary blank line.
Done
>
https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-se...
> 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-se...
> 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#newco...
> scm/chord-entry.scm:23:
> Unnecessary new line.
Done
>
https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newco...
> 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#newco...
> 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#newco...
> 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#newco...
> 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#newco...
> 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#newco...
> 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#newco...
> 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#newco...
> 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#newco...
> 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#newco...
> 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/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/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/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/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/chord-ignatzek-names.scm:379:
> Whitespace to remove.
>
>
https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names....
> 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/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#newcod...
> 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#newcod...
> 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#newcod...
> 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#newcod...
> 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#newcod...
> 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
On 2018/11/10 19:44:47, pwm wrote:
> Hi Charles, Today I built and ran 'make check' with your patch applied to
> current master. I was able to get it to pass 'make check' by making the
> following two changes.
>
> 1. In `chord-entry.scm` line 267, remove `(write-me "base3: " bass)`.
>
> 2. In that same file, line 100, remove the parens to change
`(chord-semantics)`
> to just `chord-semantics`.
>
> The first change fixed this error (but note the type check warning):
>
> input/regression/chord-names-languages.ly'
>
> ~~~
> Parsing...
> Renaming input to:
> `/home/james/lilypond-git/input/regression/chord-names-languages.ly'
> warning: type check for `bass' failed; value `(#t . #t)' must be of type
> `boolean'
>
/home/james/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:267:59:
> In procedure memoization in expression (if ba
> ss (write-me "base3: " bass) ...):
>
/home/james/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:267:59:
> In file "/home/james/lilypond-git/build/out/s
> hare/lilypond/current/scm/chord-entry.scm", line 266: Missing or extra
> expression in (if bass (write-me "base3: " bass) (list (make
> -note-ev bass (quote bass) #t)) (quote ())).
> ~~~
>
> And the second change fixed this error:
>
> input/regression/chord-name-entry.ly
>
> ~~~
> $ /home/dev/lilypond-git/build/out/bin/lilypond
> input/regression/chord-name-entry.ly
> GNU LilyPond 2.21.0
> Processing `input/regression/chord-name-entry.ly'
>
Parsing.../home/dev/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:100:35:
> In expression (chord-semantics):
>
/home/dev/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:100:35:
> Wrong type to apply: ((modifier . #f) (root . #<Pitch c' >) (extension . 7)
> (additions) (removals) (bass . #f))
> ~~~
>
> So if you have a chance to upload a new patch set with those two changes, that
> should get things moving forward with the code review process.
>
> Cheers,
> -Paul
Thanks for figuring this out. I'm now working on make check, and will post a
new patch shortly (I hope).
>
> 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
Issue 337870043: Charles Winston's GSoC code: Chord Semantics
Created 6 years, 5 months ago by chazwins6
Modified 5 years ago
Reviewers: pkx166h, lilypond-pkx, pwm, Carl
Base URL:
Comments: 31
