GSoC 2020: Unexpected "regressions"...

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

GSoC 2020: Unexpected "regressions"...

Owen Lamb
Hi all,

I've encountered a couple of unexpected regressions for my first commit.
The first one was an issue with tablature stems, which I was able to fix.
The other, however, is baffling to me.

It appears that musicxml2ly creates slightly different .ly files between
the two commits, for musicxml tests 03b and 71d. In particular, it's
confusing the order of two voices within a staff. The odd thing is that
test 71d appears to have correct output only *after *my commit is applied,
while 03b is correct only *before *my commit is applied (i.e. on the
baseline commit).

None of my code did a thing to musicxml, so I'm utterly confused. Is there
some inconsistency with musicxml2ly that may have caused this regression?

I'd appreciate it if anyone has time to do a regression test on their own
machine, just to make sure it's not just a problem with my system. I was
comparing f93efe2a (baseline, last commit before mine) and 8df138ae (my
first commit).

Thanks,
Owen
Reply | Threaded
Open this post in threaded view
|

Re: GSoC 2020: Unexpected "regressions"...

Michael Käppler-2
Am 28.08.2020 um 09:03 schrieb Owen Lamb:

> Hi all,
>
> I've encountered a couple of unexpected regressions for my first commit.
> The first one was an issue with tablature stems, which I was able to fix.
> The other, however, is baffling to me.
>
> It appears that musicxml2ly creates slightly different .ly files between
> the two commits, for musicxml tests 03b and 71d. In particular, it's
> confusing the order of two voices within a staff. The odd thing is that
> test 71d appears to have correct output only *after *my commit is applied,
> while 03b is correct only *before *my commit is applied (i.e. on the
> baseline commit).
>
> None of my code did a thing to musicxml, so I'm utterly confused. Is there
> some inconsistency with musicxml2ly that may have caused this regression?
>
> I'd appreciate it if anyone has time to do a regression test on their own
> machine, just to make sure it's not just a problem with my system. I was
> comparing f93efe2a (baseline, last commit before mine) and 8df138ae (my
> first commit).
Will run the tests on my machine today and report the results to the
list ASAP.

Michael

Reply | Threaded
Open this post in threaded view
|

Re: GSoC 2020: Unexpected "regressions"...

Michael Käppler-2
Am 28.08.2020 um 09:53 schrieb Michael Käppler:

> Am 28.08.2020 um 09:03 schrieb Owen Lamb:
>> Hi all,
>>
>> I've encountered a couple of unexpected regressions for my first commit.
>> The first one was an issue with tablature stems, which I was able to
>> fix.
>> The other, however, is baffling to me.
>>
>> It appears that musicxml2ly creates slightly different .ly files between
>> the two commits, for musicxml tests 03b and 71d. In particular, it's
>> confusing the order of two voices within a staff. The odd thing is that
>> test 71d appears to have correct output only *after *my commit is
>> applied,
>> while 03b is correct only *before *my commit is applied (i.e. on the
>> baseline commit).
>>
>> None of my code did a thing to musicxml, so I'm utterly confused. Is
>> there
>> some inconsistency with musicxml2ly that may have caused this
>> regression?
>>
>> I'd appreciate it if anyone has time to do a regression test on their
>> own
>> machine, just to make sure it's not just a problem with my system. I was
>> comparing f93efe2a (baseline, last commit before mine) and 8df138ae (my
>> first commit).
> Will run the tests on my machine today and report the results to the
> list ASAP.
I cannot reproduce this on my machine.
What I see are differences in input/regression/tablature-letter.ly
(which I would describe as slightly different spacing)
and input/regression/palm-mute.ly, where
some glyph is missing.
The log file says

warning: Cannot find glyph noteheads.u2do

HTH,
Michael

Reply | Threaded
Open this post in threaded view
|

Re: GSoC 2020: Unexpected "regressions"...

Han-Wen Nienhuys-3
In reply to this post by Owen Lamb
On Fri, Aug 28, 2020 at 9:03 AM Owen Lamb <[hidden email]> wrote:

>
> Hi all,
>
> I've encountered a couple of unexpected regressions for my first commit.
> The first one was an issue with tablature stems, which I was able to fix.
> The other, however, is baffling to me.
>
> It appears that musicxml2ly creates slightly different .ly files between
> the two commits, for musicxml tests 03b and 71d. In particular, it's
> confusing the order of two voices within a staff. The odd thing is that
> test 71d appears to have correct output only *after *my commit is applied,
> while 03b is correct only *before *my commit is applied (i.e. on the
> baseline commit).

I have seen this too. I suspect there is some hash table randomization
in the xml2ly code that makes the result unpredictable. It's not
related to the fonts, so I'd just ignore it for now.

--
Han-Wen Nienhuys - [hidden email] - http://www.xs4all.nl/~hanwen

Reply | Threaded
Open this post in threaded view
|

Re: GSoC 2020: Unexpected "regressions"...

Owen Lamb
In reply to this post by Michael Käppler-2
On Fri, Aug 28, 2020 at 3:00 AM Michael Käppler <[hidden email]> wrote:

> I cannot reproduce this on my machine.
>

All right. Thanks for giving it a try. Han-Wen, I'll take your advice and
ignore this issue for the time being.


> What I see are differences in input/regression/tablature-letter.ly
> (which I would describe as slightly different spacing)
> and input/regression/palm-mute.ly, where
> some glyph is missing.
> The log file says
>
> warning: Cannot find glyph noteheads.u2do
>

Ah! It looks like the issue is in line 12 of palm-mute.ly:
    e8^\markup { \musicglyph "noteheads.u2do"  = palm mute }
Here, \musicglyph is looking for noteheads.u2do, which I combined with
.d2do to make .s2do. Changing the string to "noteheads.s2do" makes this
regression test work as expected again.

(Is it permissible to simply update this regtest as part of the main
commit, or is there a better way to do it?)

Thanks for the help!
--Owen
Reply | Threaded
Open this post in threaded view
|

Re: GSoC 2020: Unexpected "regressions"...

Werner LEMBERG

> Ah! It looks like the issue is in line 12 of palm-mute.ly:
>
>     e8^\markup { \musicglyph "noteheads.u2do"  = palm mute }
>
> Here, \musicglyph is looking for noteheads.u2do, which I combined
> with .d2do to make .s2do.  Changing the string to "noteheads.s2do"
> makes this regression test work as expected again.
>
> (Is it permissible to simply update this regtest as part of the main
> commit, or is there a better way to do it?)

If you mean you should simply add another commit to your branch to fix
this, then yes :-)


    Werner

Reply | Threaded
Open this post in threaded view
|

Re: GSoC 2020: Unexpected "regressions"...

Owen Lamb
On Sat, Aug 29, 2020 at 2:39 AM Werner LEMBERG <[hidden email]> wrote:

>
> > Ah! It looks like the issue is in line 12 of palm-mute.ly:
> >
> >     e8^\markup { \musicglyph "noteheads.u2do"  = palm mute }
> >
> > Here, \musicglyph is looking for noteheads.u2do, which I combined
> > with .d2do to make .s2do.  Changing the string to "noteheads.s2do"
> > makes this regression test work as expected again.
> >
> > (Is it permissible to simply update this regtest as part of the main
> > commit, or is there a better way to do it?)
>
> If you mean you should simply add another commit to your branch to fix
> this, then yes :-)
>

Ah. No, I meant re-committing my first commit with the fix, then rebasing
the dev/lamb/GSoC-2020-final branch on top of it to keep my history pretty.

Would you rather I just made a new commit?

--Owen
Reply | Threaded
Open this post in threaded view
|

Re: GSoC 2020: Unexpected "regressions"...

Carl Sorensen-3
As long as all of your commits build successfully, it's not a big deal if there is a regression problem in the middle, IMO.  I'd just add a commit at the end and call it good.

Carl


On 8/29/20, 5:50 PM, "lilypond-devel on behalf of Owen Lamb" <lilypond-devel-bounces+carl.d.sorensen+digest=[hidden email] on behalf of [hidden email]> wrote:

On Sat, Aug 29, 2020 at 2:39 AM Werner LEMBERG <[hidden email]> wrote:

>
> > Ah! It looks like the issue is in line 12 of palm-mute.ly:
> >
> >     e8^\markup { \musicglyph "noteheads.u2do"  = palm mute }
> >
> > Here, \musicglyph is looking for noteheads.u2do, which I combined
> > with .d2do to make .s2do.  Changing the string to "noteheads.s2do"
> > makes this regression test work as expected again.
> >
> > (Is it permissible to simply update this regtest as part of the main
> > commit, or is there a better way to do it?)
>
> If you mean you should simply add another commit to your branch to fix
> this, then yes :-)
>

Ah. No, I meant re-committing my first commit with the fix, then rebasing
the dev/lamb/GSoC-2020-final branch on top of it to keep my history pretty.

Would you rather I just made a new commit?

--Owen

Reply | Threaded
Open this post in threaded view
|

Re: GSoC 2020: Unexpected "regressions"...

Owen Lamb
On Sat, Aug 29, 2020 at 5:07 PM Carl Sorensen <[hidden email]> wrote:

> As long as all of your commits build successfully, it's not a big deal if
> there is a regression problem in the middle, IMO.  I'd just add a commit at
> the end and call it good.


OK. (I already did it the other way, but for any other fixes that come up
I'll do it that way.)

Thanks,
Owen
Reply | Threaded
Open this post in threaded view
|

Re: GSoC 2020: Unexpected "regressions"...

Werner LEMBERG
In reply to this post by Owen Lamb

> I meant re-committing my first commit with the fix, then rebasing
> the dev/lamb/GSoC-2020-final branch on top of it to keep my history
> pretty.

In general, this is the ideal solution, since it cleans up your
branch.  However, ...

> Would you rather I just made a new commit?

... if tweaking is too complicated inspite of the support given by
git, a simple add-on commit is just fine.  Note that the GSoC deadline
means a freeze to your branch; for ongoing work you should then copy
your branch to another one and use this for further work (patches,
rebasing, etc.)


    Werner