Issue 4126042: Add Modal transformations

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+445 lines, -11 lines)			Patch
M	Documentation/notation/pitches.itely	View	1 2 3 4 5	4 chunks	+163 lines, -2 lines	6 comments	Download
A	input/regression/modal-transforms.ly	View	1	1 chunk	+34 lines, -0 lines	1 comment	Download
M	ly/music-functions-init.ly	View	1	3 chunks	+30 lines, -9 lines	3 comments	Download
M	scm/lily.scm	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	scm/modal-transforms.scm	View	1	1 chunk	+217 lines, -0 lines	4 comments	Download

http://codereview.appspot.com/4126042/diff/5002/scm/modal-transforms.scm File scm/modal-transforms.scm (right): http://codereview.appspot.com/4126042/diff/5002/scm/modal-transforms.scm#newcode157 scm/modal-transforms.scm:157: ;; 5 octaves from the original scale definition. In ...

13 years, 2 months ago (2011-02-03 07:44:37 UTC) #8

bernard_marcade.biz

On Wed, Feb 02, 2011 at 07:38:45PM +0000, Graham Percival wrote: > > Let me ...

13 years, 2 months ago (2011-02-03 08:24:17 UTC) #9

pkx166h

http://codereview.appspot.com/4126042/diff/5002/Documentation/notation/pitches.itely File Documentation/notation/pitches.itely (right): http://codereview.appspot.com/4126042/diff/5002/Documentation/notation/pitches.itely#newcode828 Documentation/notation/pitches.itely:828: Lilypond provides functions for applying these transformations. Do we ...

13 years, 2 months ago (2011-02-03 10:14:46 UTC) #10

Trevor Daniels

Thanks Keith http://codereview.appspot.com/4126042/diff/5002/Documentation/notation/pitches.itely File Documentation/notation/pitches.itely (right): http://codereview.appspot.com/4126042/diff/5002/Documentation/notation/pitches.itely#newcode867 Documentation/notation/pitches.itely:867: pentatonicScale = \relative c' { ges aes ...

13 years, 2 months ago (2011-02-03 10:25:23 UTC) #11

t.daniels_treda.co.uk

Keith wrote Thursday, February 03, 2011 7:22 AM > http://codereview.appspot.com/4126042/diff/5002/ly/music-functions-init.ly#newcode465 > ly/music-functions-init.ly:465: modalInversion = > ...

13 years, 2 months ago (2011-02-03 11:00:39 UTC) #12

Trevor Daniels

Thanks James http://codereview.appspot.com/4126042/diff/5002/Documentation/notation/pitches.itely File Documentation/notation/pitches.itely (right): http://codereview.appspot.com/4126042/diff/5002/Documentation/notation/pitches.itely#newcode828 Documentation/notation/pitches.itely:828: Lilypond provides functions for applying these transformations. ...

13 years, 2 months ago (2011-02-03 11:58:18 UTC) #13

t.daniels_treda.co.uk

Graham Percival wrote Wednesday, February 02, 2011 7:38 PM > IIRC, "diatonic" can refer to ...

13 years, 2 months ago (2011-02-03 12:43:22 UTC) #14

benko.pal

>> IIRC, "diatonic" can refer to any church mode. >> >> Let me rephrase / ...

13 years, 2 months ago (2011-02-03 15:28:51 UTC) #15

t.daniels_treda.co.uk

Benkő Pál wrote Thursday, February 03, 2011 3:28 PM > I think for these purposes ...

13 years, 2 months ago (2011-02-03 17:39:55 UTC) #16

benko.pal

>> I think for these purposes all church modes are equivalent. >> the different minor ...

13 years, 2 months ago (2011-02-03 20:14:32 UTC) #17

Graham Percival (old account)

LGTM. I don't think you need to wait until Sat. http://codereview.appspot.com/4126042/diff/3009/Documentation/notation/pitches.itely File Documentation/notation/pitches.itely (right): http://codereview.appspot.com/4126042/diff/3009/Documentation/notation/pitches.itely#newcode840 ...

13 years, 2 months ago (2011-02-04 01:43:38 UTC) #18

benko.pal

> http://codereview.appspot.com/4126042/diff/3009/Documentation/notation/pitches.itely#newcode972 > Documentation/notation/pitches.itely:972: Manual ties inside > @code{\retrograde} will be broken and > not ...

13 years, 2 months ago (2011-02-04 08:31:13 UTC) #19

Neil Puttock

http://codereview.appspot.com/4126042/diff/3009/Documentation/notation/pitches.itely File Documentation/notation/pitches.itely (right): http://codereview.appspot.com/4126042/diff/3009/Documentation/notation/pitches.itely#newcode914 Documentation/notation/pitches.itely:914: octotonicScale = \relative c' { ees f fis gis ...

13 years, 2 months ago (2011-02-04 10:24:26 UTC) #20

benko.pal

http://codereview.appspot.com/4126042/diff/3009/scm/modal-transforms.scm File scm/modal-transforms.scm (right): http://codereview.appspot.com/4126042/diff/3009/scm/modal-transforms.scm#newcode68 scm/modal-transforms.scm:68: (lambda (pivot-pitch pitch) I'm afraid this is not a ...

13 years, 2 months ago (2011-02-04 17:11:36 UTC) #21

t.daniels_treda.co.uk

Pal wrote Friday, February 04, 2011 5:11 PM > http://codereview.appspot.com/4126042/diff/3009/scm/modal-transforms.scm#newcode68 > scm/modal-transforms.scm:68: (lambda (pivot-pitch pitch) ...

13 years, 2 months ago (2011-02-05 11:28:40 UTC) #22

Trevor Daniels

I had to start a new branch and Rietveld number. See http://codereview.appspot.com/4079064/ for the continuation ...

13 years, 2 months ago (2011-02-05 12:04:07 UTC) #23

t.daniels_treda.co.uk

Changes made in http://codereview.appspot.com/4079064/ ----- Original Message ----- From: <n.puttock@gmail.com> Sent: Friday, February 04, 2011 ...

13 years, 2 months ago (2011-02-05 12:09:20 UTC) #24

t.daniels_treda.co.uk

Changes in http://codereview.appspot.com/4079064/ From: <percival.music.ca@gmail.com> Sent: Friday, February 04, 2011 1:43 AM Subject: Re: Add ...

13 years, 2 months ago (2011-02-05 12:14:16 UTC) #25

michael.f.ellis

Thanks. This is good feedback. I agree that an extra argument could be useful. I ...

13 years, 2 months ago (2011-02-05 16:23:55 UTC) #26

Thanks. This is good feedback.  I agree that an extra argument could
be useful.  I think of this operation as transposition followed by
inversion,  so Pal's example could be implemented this way:

%% ------------------------------------------------
modalTransposedInversion =
#(define-music-function (p l from to-pivot scale music)
   (ly:music? ly:music? ly:music? ly:music? )
     #{
        \modalInversion $to-pivot $scale \modalTranspose $from
$to-pivot $scale $music
     #})

scale = {c g}
original = { c4. c'8 g'4. g8 }
desired =  { g4. g,8 c,4. c8 }
{
    \clef bass
    \original
    \desired
    \modalTransposedInversion c g \scale \original

}
%% ------------------------------------------------

So it could be left to the user, but I imagine that transposition will
be wanted more often than not,  so I have no problem with altering the
definitions accordingly.

In practice, it will be more efficient to code it as Pal suggests,
with two index operations -- although I have no idea whether the
efficiency gain would be significant in terms of LilyPond's total
processing overhead.

I do like arranging and naming the arguments as suggested above.  I
think it will simplify documentation and usage if the arguments to
\modalTranspose and \modalInversion (or \modalTransposedInversion if
you prefer)  are as congruent as possible, so that we can have

\modalTranspose   from-pitch   to-pitch  ...
and
\modalInversion     from-pitch    to-pivot  ...

I also find that this corresponds to the way I think about it
musically.  I see the original sequence, then imagine where I want the
first pitch of the inverted sequence to be.

Does anyone have a strong feeling whether we should rename
\modalInversion to \modalTransposedInversion?  Or perhaps leave
\modalInversion as is and offer \modalTransposedInversion as a
separate function?

Daniel, what's the best way for me to submit the changes?  Does
codereview support a way to locally clone the current version.  And
would you rather have a patch or a new copy?

Cheers,
Mike

On Sat, Feb 5, 2011 at 6:28 AM, Trevor Daniels <t.daniels@treda.co.uk> wrote:
>
> Pal wrote Friday, February 04, 2011 5:11 PM
>
>>
http://codereview.appspot.com/4126042/diff/3009/scm/modal-transforms.scm#newc...
>> scm/modal-transforms.scm:68: (lambda (pivot-pitch pitch)
>> I'm afraid this is not a good interface, as there are inversions where
>> the pivot is between two notes.
>>
>> e.g. with the scale { c g } I may want to invert
>> { c4. c'8 g'4. g8 } to get { g4. g,8 c,4. c8 }.
>
> Good point!
>
>> I see two ways out:
>> 1. have a source-pitch and a target-pitch, like transposition
>> 2. have a single pitch, which is not a pivot, but the image of the
>> starting pitch in the scale definition.
>
> There is a third way, which I would prefer.  An inversion
> can pivot around a note in the scale, as now, or around a
> point between two adjacent notes in the scale, which is
> missing from the current implementation.
>
> I would suggest permitting the pivot to be either one
> note, as now, or two notes, in which case the pivot is
> a point between the two.
>
>>
http://codereview.appspot.com/4126042/diff/3009/scm/modal-transforms.scm#newc...
>> scm/modal-transforms.scm:83: (index pitch scale)))
>> With my suggestion 1. above these three lines could be written as
>>
>> (+ (index target-pitch scale)
>>   (- (index source-pitch scale)
>>      (index pitch scale)))
>
> With my suggestion your example would then be coded:
>
>  \modalInversion { c g } {c g} { c4. c'8 g'4. g8 }
>
> @Mike: do you see any problems with this?  Would you be
> able to code this up?  I can fix the docs.
>
>> http://codereview.appspot.com/4126042/
>
> Trevor
>
>

benko.pal

> Thanks. This is good feedback. I agree that an extra argument could > be ...

13 years, 2 months ago (2011-02-05 17:24:59 UTC) #27

> Thanks. This is good feedback.  I agree that an extra argument could
> be useful.  I think of this operation as transposition followed by
> inversion,  so Pal's example could be implemented this way:
>
> %% ------------------------------------------------
> modalTransposedInversion =
> #(define-music-function (p l from to-pivot scale music)
>   (ly:music? ly:music? ly:music? ly:music? )
>     #{
>        \modalInversion $to-pivot $scale \modalTranspose $from
> $to-pivot $scale $music
>     #})
>
> scale = {c g}
> original = { c4. c'8 g'4. g8 }
> desired =  { g4. g,8 c,4. c8 }
> {
>    \clef bass
>    \original
>    \desired
>    \modalTransposedInversion c g \scale \original
>
> }
> %% ------------------------------------------------
>
> So it could be left to the user, but I imagine that transposition will
> be wanted more often than not,  so I have no problem with altering the
> definitions accordingly.

yes; I just thought that practically the same code can
be used to get the final result, so why not.

> In practice, it will be more efficient to code it as Pal suggests,
> with two index operations -- although I have no idea whether the
> efficiency gain would be significant in terms of LilyPond's total
> processing overhead.

by the way efficiency: my automatic reaction is that
validity check should be moved out from the lambda
expression into the user level function (or just one
level below), but I have absolutely no experience.

> I do like arranging and naming the arguments as suggested above.  I
> think it will simplify documentation and usage if the arguments to
> \modalTranspose and \modalInversion (or \modalTransposedInversion if
> you prefer)  are as congruent as possible, so that we can have
>
> \modalTranspose   from-pitch   to-pitch  ...
> and
> \modalInversion     from-pitch    to-pivot  ...
>
> I also find that this corresponds to the way I think about it
> musically.  I see the original sequence, then imagine where I want the
> first pitch of the inverted sequence to be.
>
> Does anyone have a strong feeling whether we should rename
> \modalInversion to \modalTransposedInversion?  Or perhaps leave
> \modalInversion as is and offer \modalTransposedInversion as a
> separate function?

to me inversion is enough, since, as you say, changing
the pivot (whether it's a scale element or not) means
transposition, i.e. transposition is unseparable from
inversion.

p

t.daniels_treda.co.uk

Michael Ellis wrote Saturday, February 05, 2011 4:23 PM > In practice, it will be ...

13 years, 2 months ago (2011-02-05 18:45:47 UTC) #28

michael.f.ellis

13 years, 2 months ago (2011-02-14 21:46:28 UTC) #29

Hi Trevor,
Took me a couple of days longer to get to this than I planned.  Sorry
for the delay.  Zip of new versions attached.  Believe it addresses
all issues from rietveld.

-- All scheme functions now have docstrings.
-- All ly:warnings have (_i "warning text")
-- All module names begin with "modal-transforms"
-- \modalInversion now has 2 required pitch arguments per our discussions.

I've also made the argument naming consistent throughout the scheme
and music functions.

Transpose is now
\modalTranspose from to scale music

and inversion is now
\modalInversion around to scale music

so that
\displayLilyMusic \modalInversion e' f'  {c d e f g a b} { c'4 e' g' }

yields {a' f' d'},

i.e. modally inverted "around" e' and then modally transposed
so that e' moves "to"  f'.

Notice that modalInversion  has the argument order changed since our
last discussion  where were considering  "from  pivot" because as I
played with the functions I found it slightly confusing to mentally
map the outcome to the arguments.  I think the new names and order are
easier to think about (in English at least).  Hope this seems ok to
everyone.
Again, thanks for your encouragement and help!

Cheers,
Mike

On Sat, Feb 5, 2011 at 1:45 PM, Trevor Daniels <t.daniels@treda.co.uk> wrote:
>
> Michael Ellis wrote Saturday, February 05, 2011 4:23 PM
>
>> In practice, it will be more efficient to code it as Pal suggests,
>> with two index operations -- although I have no idea whether the
>> efficiency gain would be significant in terms of LilyPond's total
>> processing overhead.
>
> I'm not unhappy with that.  And you're right, the symmetry
> of the operations is then quite pleasing.
>
>> Daniel, what's the best way for me to submit the changes?  Does
>> codereview support a way to locally clone the current version. And
>> would you rather have a patch or a new copy?
>
> Trevor, actually :)
>
> You can download the patchset from Rietfeld using the
> "download raw patch set" button.  It should apply to
> any recent state of git.
>
> A patch against git with this download applied will be
> fine, as would a new copy of the changed files.
>
> Trevor
>
>
>

Expand All Messages | Collapse All Messages