RootSystem __init__ builds the dual twice, breaking initialization of non-crystallographic root systems

[darijgr]

Relevant piece of the code:

        if as_dual_of is None:
            self.dual_side = False
            self.dual = RootSystem(self._cartan_type.dual(), as_dual_of=self);
            # still fails for CartanType G2xA1
            try:
                self.dual = RootSystem(self._cartan_type.dual(), as_dual_of=self);
            except StandardError:
                pass

The definition of self.dual is done twice, one time in a try clause, one time outside. I don't know if the breaking of non-crystallographic root systems is intentional or not (might be it is because there doesn't seem to be much functionality implemented for that), but it certainly slows down things.

CC: @anneschilling @tscrim @sagetrac-sage-combinat @nthiery @mwhansen @dwbump

Component: combinatorics

Keywords: root-system, sage-combinat

Author: Darij Grinberg

Reviewer: Travis Scrimshaw

Merged: sage-5.13.beta1

Issue created by migration from https://trac.sagemath.org/ticket/15279

[darijgr]

Attachment: trac_15279-init_RootSystem-dg.patch.gz

[tscrim]

comment:3

Looks good to me.

[tscrim]

Reviewer: Travis Scrimshaw

[nthiery]

comment:4

This sounds like a merge that went wrong, so +1 on the change. You might want to add a test if this impacts non crystalographic root systems (yes we want to progressively support them!!!).

In the long run, we would want something better than just trying and hoping for the best. We should be able to tell exactly when the dual should be defined or not.

[darijgr]

comment:5

I don't know what to test for the non-crystallographic ones, since nothing nontrivial seems to work for them so far... Once they are actually implemented, they will have their own doctests, which of course will implicitly test __init__ as well, so I'm not worried about init. As for now, this is more about speed and just getting rid of a line that doesn't make sense.

Travis: thanks for looking at it!

[jdemeyer]

comment:6

Replying to @nthiery:

This sounds like a merge that went wrong

It would be interesting to know why/how it went wrong. The patch is from #5794, 4 years ago.

Interestingly, the patch attachment: trac_5794-exceptional.patch:ticket:5794 on Trac is correct, it does remove the duplicate line. However, the patch which is merged (revision -r13366 in Mercurial) has a different commit message (trac_5794-exceptional.patch modified for combinat server) and does not remove that line. I can only hope that the "combinat server" doesn't make these mistakes any more.

[jdemeyer]

comment:7

There is also this suspicious commit, which was never actually reverted.

changeset:   13363:359efb582d39
user:        Mike Hansen <[email protected]>
date:        Thu Nov 19 08:25:56 2009 -0800
summary:     this temporary patch to be taken down when root system and 5794 patches stabilize.

[jdemeyer]

comment:8

More generally, the patches on Trac from #5794 are not the ones which were merged. I don't plan to further investigate this, but perhaps somebody should.

[darijgr]

comment:9

Is there a way to see the patches which were merged? This is spooky. Thanks for getting to the roots (oops) of this, Jeroen!

[jdemeyer]

comment:10

Replying to @darijgr:

Is there a way to see the patches which were merged?

Use hg log and hg export -r13366 to see a particular commit.

[nthiery]

comment:11

Replying to @jdemeyer:

Replying to @nthiery:

This sounds like a merge that went wrong

It would be interesting to know why/how it went wrong. The patch is from #5794, 4 years ago.

Interestingly, the patch attachment: trac_5794-exceptional.patch:ticket:5794 on Trac is correct, it does remove the duplicate line. However, the patch which is merged (revision -r13366 in Mercurial) has a different commit message (trac_5794-exceptional.patch modified for combinat server) and does not remove that line. I can only hope that the "combinat server" doesn't make these mistakes any more.

Not having duplication between patches on the combinat server and on trac and not having to rebase constantly, like we will have with the new workflow, will certainly help ...

Thanks for tracking this down!

[jdemeyer]

comment:12

Replying to @nthiery:

not having to rebase constantly, like we will have with the new workflow

Why would one need to rebase less with the new workflow? A merge conflict is a merge conflict, and the new workflow is not going to change that.

[nthiery]

comment:13

Replying to @jdemeyer:

Replying to @nthiery:

not having to rebase constantly, like we will have with the new workflow

Why would one need to rebase less with the new workflow? A merge conflict is a merge conflict, and the new workflow is not going to change that.

Granted, there will be as many merges as before; but unlike what we had with our patch queues those will be 3-way merges, so there will be better support from the revision control system, with less error-prone manual operations.

Cheers,

[jdemeyer]

Merged: sage-5.13.beta1

[darijgr]

comment:15

I am CCing Daniel Bump and Mike Hansen; maybe they can shed more light on the question what exactly ended up merged from #5794.

So these seem to be the #5794-related commits in the log (thank you, Jeroen, for the help with getting it):

changeset:   13469:7d776d3652ea
user:        Nicolas M. Thiery <[email protected]>
date:        Thu Nov 19 12:23:11 2009 +0100
summary:     [mq]: trac_5794-long-time-nt.patch

[...]

changeset:   13368:2fcea95a7e9c
user:        Mike Hansen <[email protected]>
date:        Thu Nov 19 08:26:08 2009 -0800
summary:     ReST fixes and improvements

changeset:   13367:4a11faf380b3
user:        Daniel Bump <[email protected]>
date:        Wed May 20 13:55:19 2009 -0700
summary:     Further exceptional branching rules

changeset:   13366:ffe6380fbc20
user:        Daniel Bump <[email protected]>
date:        Tue May 12 21:56:35 2009 -0700
summary:     trac_5794-exceptional.patch modified for combinat server

changeset:   13365:a28740f742ec
user:        Mike Hansen <[email protected]>
date:        Thu Nov 19 08:26:00 2009 -0800
summary:     imported patch trac_5794-continued-combinat

changeset:   13364:e75cad2172eb
user:        Mike Hansen <[email protected]>
date:        Thu Nov 19 08:25:57 2009 -0800
summary:     imported patch cartan_type_temporary-1.patch

changeset:   13363:359efb582d39
user:        Mike Hansen <[email protected]>
date:        Thu Nov 19 08:25:56 2009 -0800
summary:     this temporary patch to be taken down when root system and 5794 patches stabilize.

changeset:   13362:3fef40a05bb4
user:        Daniel Bump <[email protected]>
date:        Wed May 06 13:17:39 2009 -0700
summary:     trac_5794-revised.patch modified for combinat server

I can tell that

changeset 13469 == trac_5794-long-time-nt.patch (modulo fuzz)

and

changeset 13368 == trac_5794-reviewer-nt.patch.

Furthermore,

changeset 13367 differs from trac_5794-more-exceptional.patch only in having is_irreducible and is_reducible in lieu of is_atomic and is_compound.

The difference between changeset 13366 and trac_5794-exceptional.patch is more substantial, and is what caused the bug in the present ticket. Someone else should look into the diff to check if this is the only issue caused!

The difference between changeset 13365 and trac_5794-continued.patch is again only in irreducible-vs-atomic and reducible-vs-compound.

Changeset 13364 is not an attachment on #5794 and all it does is replacing some "reducible" by "compound" resp. "irreducible" by "atomic".

Changeset 13363 ("this temporary patch to be taken down when root system and 5794 patches stabilize.") seems not to be from the #5794 attachments either, and I don't really understand what it does. I don't know if anyone has ever "taken it down" or reverted its edits.

Changeset 13362 has noticeable differences from trac_5794-revised.patch and maybe someone should look into that.