Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement absolute length fix #37001

Merged
merged 16 commits into from
Jan 22, 2024
Merged

Conversation

thecaligarmo
Copy link
Contributor

Added a fix for absolute length which was previously giving wrong information. We used Theorem 1.1 from Dyer's paper "On Minimal Lengths of Expressions of Coxeter Group Elements as Products of Reflections" as suggested by Travis Scrimshaw.

Fixes #36174

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

#36174

@tscrim tscrim self-requested a review January 3, 2024 12:49
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I have two nitpicks (which you can chose to ignore) and one potential optimization if you want to see if it will work. Essentially by using the strong exchange condition, we can build all potential reflections (from the left):

w = self.reduced_word()
if len(w) <= 2:  # trivial cases
    return ZZ(len(w))
P = self.parent()
one = P.one()
reflections = []
for i in range(len(w)):
    refl = P.from_reduced_word(w[:i+1] + list(reversed(w[:i])))
    reflections.append(refl)
for ell in range(len(w)):
    for chain in itertools.combinations(reflections, ell):
        if prod(chain) == w:
            return ell

I suspect this will be faster for things of longer absolute length because it should do fewer multiplications. Anyways, it's up to you if you want to pursue any optimizations.

src/doc/en/reference/references/index.rst Outdated Show resolved Hide resolved
src/doc/en/reference/references/index.rst Outdated Show resolved Hide resolved
thecaligarmo and others added 2 commits January 3, 2024 15:30
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@thecaligarmo
Copy link
Contributor Author

Thank you. I have two nitpicks (which you can chose to ignore) and one potential optimization if you want to see if it will work. Essentially by using the strong exchange condition, we can build all potential reflections (from the left):

w = self.reduced_word()
if len(w) <= 2:  # trivial cases
    return ZZ(len(w))
P = self.parent()
one = P.one()
reflections = []
for i in range(len(w)):
    refl = P.from_reduced_word(w[:i+1] + list(reversed(w[:i])))
    reflections.append(refl)
for ell in range(len(w)):
    for chain in itertools.combinations(reflections, ell):
        if prod(chain) == w:
            return ell

I suspect this will be faster for things of longer absolute length because it should do fewer multiplications. Anyways, it's up to you if you want to pursue any optimizations.

Wouldn't this take roughly the same amount of time? Mainly because, in both cases, the thing that's going to take the longest amount of time is the itertools.combinations part. In fact, this would probably take a little longer because we first need to grab all reflections. The product would be faster since there's less terms, but I think this would be roughly equivalent to the already created algorithm?

@tscrim
Copy link
Collaborator

tscrim commented Jan 3, 2024

Yes, although I realized we can generate the possible reflections linearly in $\ell(w)$ number of multiplications (instead of the quadratic I previously said):

s = P.simple_reflections()
rev = P.one()
cur = P.one()
reflections = []
for val in w:
    cur = cur * s[val]
    reflections.append(cur * rev)
    rev = s[val] * rev

I suspect this will be faster when the absolute length is longer. It will almost certainly be slower when the absolute length is short. Well, either way it might be good to implement this as absolute_chain().

In a related note, we should also fix the infinite loop on absolute_covers(). We could simply move the method from the category of all Coxeter groups to finite Coxeter groups as I suspect there is always an infinite number of covers when the group is infinite. The other option is would be to change the output to an iterator (and absorb some backwards incompatibility).

I know it is PR creep, but it would be good to fix all of the issues with absolute order for infinite Coxeter groups all at once.

@thecaligarmo
Copy link
Contributor Author

But the for loop afterwards is still basically the same time length as the one currently implemented. You still need two for loops and one of them uses combinations. It seems like this should roughly be the same amount of time. (Or maybe I'm not understanding?)

r.e absolute_covers(), I think it's worthwhile leaving it, but converting it to an iterator makes sense. It's a small fix, so I've gone ahead and done it. I ran the test suite on all files in categories and in combinat/root_system (which should be everything that touches this?) and fixed the errors that showed up (there was just 1)

@thecaligarmo
Copy link
Contributor Author

Looks like other tests in the combinat directory are failing. I'll run a more thorough test and see what else needs fixing. Hopefully it's not to much or else I might just revert it and say we should seperate these into two tickets to not have PR creep.

@thecaligarmo
Copy link
Contributor Author

Should be all good now. I ran all the tests of sage, and it looks like it's ok.

An alternative approach would be maybe to return a list if it's finite and a generator otherwise? Or would this be to confusing because the return type changes depending on if the group is finite/infinite. (But then it would be backwards compatible, so there is that?)

@tscrim
Copy link
Collaborator

tscrim commented Jan 4, 2024

But the for loop afterwards is still basically the same time length as the one currently implemented. You still need two for loops and one of them uses combinations. It seems like this should roughly be the same amount of time. (Or maybe I'm not understanding?)

You're right that the two main loops are the same. However, the key point is the number of multiplications in the Coxeter group. The current way it's implemented requires ell multiplications to compute from_reduced_word, which is $O(\ell)$. So it should take a lot longer if the absolute length equals $\ell(w)$.

Should be all good now. I ran all the tests of sage, and it looks like it's ok.

Great! Thank you.

An alternative approach would be maybe to return a list if it's finite and a generator otherwise? Or would this be to confusing because the return type changes depending on if the group is finite/infinite. (But then it would be backwards compatible, so there is that?)

I think we should just absorb any pain with the backwards incompatible change. It's somewhat difficult to deprecate and at some point, people will just have to deal with the switch (and it often happens that people have sufficiently large version jumps that render such things moot). On the other hand, I think it wouldn't be so confusing for infinite groups to be an iterator and finite to return a list since the user should expect it to be infinite in general. Although IIRC Python gets feisty when you mix non-trivial returns with iterator functions.

@thecaligarmo
Copy link
Contributor Author

Ok sounds good. I'll go ahead and make the change to the secondary version. That's what I had been asking earlier, if the product computation was "big enough" that it would cause performance differences and I think your answer above is mentioning yes.

I think we should just absorb any pain with the backwards incompatible change. It's somewhat difficult to deprecate and at some point, people will just have to deal with the switch (and it often happens that people have sufficiently large version jumps that render such things moot). On the other hand, I think it wouldn't be so confusing for infinite groups to be an iterator and finite to return a list since the user should expect it to be infinite in general. Although IIRC Python gets feisty when you mix non-trivial returns with iterator functions.

An alternative approach that I thought of yesterday night: We can create a second function absolute_covers_iter that returns the generator. Then in absolute_covers we return a list for finite and we throw a warning saying it's infinite for the infinite cases and to use absolute_cover_iter instead. How would that sound? Makes it backwards compatible, makes it so the functions always return the same type of thing, and separates out finite vs infinite. The downside would be code bloat.

@tscrim
Copy link
Collaborator

tscrim commented Jan 5, 2024

Ok sounds good. I'll go ahead and make the change to the secondary version. That's what I had been asking earlier, if the product computation was "big enough" that it would cause performance differences and I think your answer above is mentioning yes.

I think it just needs to be tested against each other to see which (generally) has the better performance. If you prefer, I can do the comparison testing.

I think we should just absorb any pain with the backwards incompatible change. It's somewhat difficult to deprecate and at some point, people will just have to deal with the switch (and it often happens that people have sufficiently large version jumps that render such things moot). On the other hand, I think it wouldn't be so confusing for infinite groups to be an iterator and finite to return a list since the user should expect it to be infinite in general. Although IIRC Python gets feisty when you mix non-trivial returns with iterator functions.

An alternative approach that I thought of yesterday night: We can create a second function absolute_covers_iter that returns the generator. Then in absolute_covers we return a list for finite and we throw a warning saying it's infinite for the infinite cases and to use absolute_cover_iter instead. How would that sound? Makes it backwards compatible, makes it so the functions always return the same type of thing, and separates out finite vs infinite. The downside would be code bloat.

I thought about that as well, but it the "list" version is an infinite loop for infinite types (which is a bug IMO). Plus, as you said, it adds some bloat and the current implementation more naturally says it should be a list. I think following the Python3 model -- just telling people it is now an iterator and to put list() around it if they want to manipulate the list -- is the most reasonable way to deal with this.

@thecaligarmo
Copy link
Contributor Author

thecaligarmo commented Jan 10, 2024

(I deleted my previous comment as I had made a mistake in my explanation. This one should be correct. Sorry.)

I just tested the code on the following word:

W = CoxeterGroup(["A", 2, 1])
(r, s, t) = W.simple_reflections()
w = (r * s * r * t)

The reflections we get back are: $r$, $rsr$, $s$, and $rsrtrsr$ which are correct, but notice that $w$ decomposes into two reflections as $w = rsrtrsr \cdot rsr$. In the code, we have itertools.combinations which never puts these in this particular order. In other words, we don't just want the combinations, we want all permutations of the combinations. Additionally, is there not a possibility that a reflection might exist more than once? (I'm thinkin $rstr$ or something similar) So we'd also want combinations with replacement. So the code would look like:

for ell in range(w.length()):
    for chain in itertools.combinations_with_replacement(reflections, ell):
        for wd in itertools.permutations(chain):
            if (P.prod() == w):
                return ell

Even if we don't need combinations_with_replacement, doing permutations instead of combinations becomes much bigger when the number of reflections get larger. For fun, I did a test run to see how long things would take.
I calculated the absolute length on all elements in: $A_2$, $A_3$, $A_4$, $A_5$, $B_2$, $B_3$, $B_4$, $B_5$, $D_2$, $D_3$, $D_4$, $D_5$. In total, we had the following times:
Dyer's method: $2,113$ seconds (Roughly $35$ minutes)
Your method: $9,347$ seconds (Roughly $155$ minutes)
If we remove the 5 dimensional Coxeter groups we get:
Dyer's method: $6$ seconds
Your method: $10$ seconds

Because of this, I'd say let's stick with Dyer's method as (although it's not optimised) it is currently the fastest approach we know based on research.

I thought about that as well, but it the "list" version is an infinite loop for infinite types (which is a bug IMO). Plus, as you said, it adds some bloat and the current implementation more naturally says it should be a list. I think following the Python3 model -- just telling people it is now an iterator and to put list() around it if they want to manipulate the list -- is the most reasonable way to deal with this.

Sounds good. Then we can just leave it as it currently is as that's how I implemented it? Made it into an iter and updated everything to use list() around the iter for the finite cases. Sounds like this should be done?

@tscrim
Copy link
Collaborator

tscrim commented Jan 10, 2024

You can edit your posts. :)

I don't think we need to change the order of the reflections, nor do we need to consider "repetitions." I put repetitions in quotes because with how the reflections are construction, we might get the same reflection appearing more than once in the list. My idea is translating removing an element from a reduced word into multiplying by a reflection. I thought this is what the strong exchange condition gives us.

Now I finally have some time to write and test the code instead of just posting thoughts here. I see a mistake in the implementation of my idea: We want to multiply things in the reverse order as I was thinking of doing $r_1 \cdots r_{\ell} w = 1$, so the products equal $w^{-1}$ and since all reflections are involutions, hence the order reversal. In fact, you can see this in your example.

That being said, I ran some timings and Dyer's method is clearly faster for most examples:

sage: W = CoxeterGroup(["A", 2, 1])
sage: it = iter(W)
sage: %time L = [next(it).absolute_length(True) for _ in range(200)]  # my proposal
CPU times: user 3.63 s, sys: 0 ns, total: 3.63 s
Wall time: 3.93 s
sage: it = iter(W)
sage: %time Lp = [next(it).absolute_length() for _ in range(200)]  # current implementation
CPU times: user 181 ms, sys: 0 ns, total: 181 ms
Wall time: 196 ms
sage: L == Lp
True

Hence, I propose adding the code I have as:

        def absolute_chain(self):
            r"""
            Return a (saturated) chain in absolute order from ``self``
            to ``1``.
            """
            if self.is_one():
                return [self]
            w = self.reduced_word()
            if len(w) == 1:  # trivial case
                return [P.one(), self]
            if len(w) == 2:  # trivial case
                return [P.one(), P.simple_reflection(w[0]), self]
            s = P.simple_reflections()
            import itertools
            from sage.misc.misc_c import prod
            rev = P.one()
            cur = P.one()
            reflections = []
            for val in w:
                cur = cur * s[val]
                reflections.append(cur * rev)
                rev = s[val] * rev
            for ell in range(len(w)):
                for chain in itertools.combinations(reflections, ell):
                    if prod(reversed(chain)) == w:
                        return (P.one(),) + chain

What do you think?

I also found the same bug in not handling 1 properly in your implementation. Please add a doctest for it and add the short special case check:

if len(w) <= 2:  # trivial cases
    from sage.rings.integer import Integer
    return Integer(len(w))

@thecaligarmo
Copy link
Contributor Author

(Yes, I know you can edit, but I didn't know how long it would take to fix the wrong information I had put, so I decided better to just delete and readd once I actually have more detailed/correct information)

Oooo I think I finally see what you're trying to do. Sorry I'm a little slow with things currently due to life circumstances. Yeah, strong exchange should work. So it sounds like absolute_chain basically gives you (a) minimal (absolute) length list of reflections which, when multiplied, give you the word. Am I understanding that correctly?

@tscrim
Copy link
Collaborator

tscrim commented Jan 11, 2024

It didn't help that I had bugs in my code. No problem on slow responses.

Yes, that is what it should do. I was surprised at how significantly slower it is, but I didn't try to profile it. Although it does give some more information in a clean way; plus having it would give access for the user in case it was faster.

@thecaligarmo
Copy link
Contributor Author

That being said, I ran some timings and Dyer's method is clearly faster for most examples

I don't know how you got yours so slow? (or the proposed method so fast?) because for me yours is clearly running faster. (abs_len_two is my test code for your proposal.)

sage: W = CoxeterGroup(['A', 2, 1])
sage: n = 2000
sage: it = iter(W)
sage: %time L = [next(it).abs_len_two() for _ in range(n)] # Travis' proposal
CPU times: user 5.25 s, sys: 0 ns, total: 5.25 s
Wall time: 5.25 s
sage: it = iter(W)
sage: %time Lp = [next(it).absolute_length() for _ in range(n)] # Original proposal
CPU times: user 57.4 s, sys: 10 ms, total: 57.5 s
Wall time: 57.5 s
sage: L == Lp
True

So what I ended up doing is doing some more tests where I looked at elements of a certain length and how long it takes (because as discussed, the longer the word, the longer the "original" solution should take and yours should be much faster) I found out that even when the length is $0$, your code is still faster. The only times the proposed code was faster was when the length was equal to $3$ (took 380 micro seconds for 9 elements instead of 418) and when the length was equal to $6$ (3.74 ms vs 4.17 ms). So I decided to just implement your code instead as it's the faster method.

Note that I did change some of your code. In particular:

  1. I made it so it always returns a list
  2. For n==2 you had just put self as the last element, but I think we want a chain of reflections, so I made it a reflection
  3. There was missing a case when ell == n, so I added one final case at the very end
  4. Used the product from the parent instead of from misc
  5. the final == should be with self and not with w as w is a list and not an element.

I added a few more examples (maybe too much?) but anyway, it's now running relatively smoothly on my end.

@tscrim
Copy link
Collaborator

tscrim commented Jan 11, 2024

Thank you. Maybe I had some stupid mistake with the implementation. Perhaps related with (5) in your changes or accidentally swapping what the extra (temporary) input value meant. Well, anyways, I will run tests on my side again tomorrow to reconfirm.

One thing I got wrong with the absolute_chain() doc is that it isn't returning the actual chain but the reflections used to generate the chain. So this is slightly different than what I wrote in my doc. It would be better to call the method absolute_chain_reflections() and then

def absolute_chain(self):
    P = self.parent()
    return [P.prod(elts[:i] for i in range(len(elts))]

and we can then drop adding [P.one()] to the "chain" and add + 1 to absolute_length(). Sorry for all the errors and stupidity on my part!

@thecaligarmo
Copy link
Contributor Author

Ok, I'll need a little bit of time to work on this to make sure it all handles nicely. I think that absolute_chain_reflections() shouldn't have P.one(), but then absolute_chain should have it since we're saying "from self to 1". So one of the elements should be self and one should be 1.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you're still working on adding the additional method and fixing my mistake with the chains, but I wanted to pass along a few other minor comments that will be needed for a positive review. Basically they are just minor nitpicks.

src/sage/categories/coxeter_groups.py Outdated Show resolved Hide resolved
src/sage/categories/coxeter_groups.py Outdated Show resolved Hide resolved
src/sage/categories/coxeter_groups.py Outdated Show resolved Hide resolved
src/sage/categories/coxeter_groups.py Outdated Show resolved Hide resolved
src/sage/categories/coxeter_groups.py Outdated Show resolved Hide resolved
@tscrim
Copy link
Collaborator

tscrim commented Jan 15, 2024

I am wondering if the finite type implementation is a lower bound on the absolute length. Although even if we knew that information, I am not sure it would help in the general case....much less if it is mathematically interesting...

Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@thecaligarmo
Copy link
Contributor Author

I just added your changes. All that was left was a double check that everything looked good, so we should be ok.

In affine case, it's true, but I don't think it will decrease performance that much unless our groups are "large" and our word is "long". I don't know if there's such a thing in other (infinite) cases. It would be useful/nice to look at, but might be a project for another day.

@tscrim
Copy link
Collaborator

tscrim commented Jan 16, 2024

Thank you. Almost there. I feel like chains should start from the smallest element and go up. This would match what the posets do:

sage: B4 = posets.BooleanLattice(4)
sage: B4.maximal_chains()[0]
[0, 8, 9, 11, 15]

Also, I just realized that we could get rid of the is_finite() check in absolute_order by moving the finite case down to the FiniteCoxeterGroups. I can do this if you agree with it (as I have superpowers).

@thecaligarmo
Copy link
Contributor Author

Thank you. Almost there. I feel like chains should start from the smallest element and go up. This would match what the posets do:

sage: B4 = posets.BooleanLattice(4)
sage: B4.maximal_chains()[0]
[0, 8, 9, 11, 15]

Sounds good. But I think for absolute_chain_reflection we should leave it as is since the idea is that multiplying should give us the reflection.

Also, I just realized that we could get rid of the is_finite() check in absolute_order by moving the finite case down to the FiniteCoxeterGroups. I can do this if you agree with it (as I have superpowers).

FiniteCoxeterGroups pulls from CoxeterGroups right? So I should just be able to create the absolute_length function in there and it should take precedence correct? Or do I need to do anything difference since it's an element class within the object?

src/sage/categories/coxeter_groups.py Outdated Show resolved Hide resolved
:meth:`absolute_length`,
:meth:`absolute_chain`

EXAMPLES::
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also have examples where the absolute length is > 2 and equal to the usual length of the element (which should also be length 3 or more) to cover all of the code paths..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my forthcoming commit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. It would also be good to have one test in absolute_length() with a length > 2 (perhaps a different one with a length > 5?).

src/sage/categories/coxeter_groups.py Outdated Show resolved Hide resolved
@tscrim
Copy link
Collaborator

tscrim commented Jan 18, 2024

Thank you. Almost there. I feel like chains should start from the smallest element and go up. This would match what the posets do:

sage: B4 = posets.BooleanLattice(4)
sage: B4.maximal_chains()[0]
[0, 8, 9, 11, 15]

Sounds good. But I think for absolute_chain_reflection we should leave it as is since the idea is that multiplying should give us the reflection.

Yes, I agree that they should be consistent in the sense of

$$(1, r_1, w_1, r_2, w_2, \ldots, r_{\ell}, w_{\ell})$$

should be the labeled path in the poset (with the odd positions being the edge label and even being the vertices) with $[r_1, \ldots, r_{\ell}]$ being the output of absolute_chain_reflections (perhaps with the s is a better name?).

Also, I just realized that we could get rid of the is_finite() check in absolute_order by moving the finite case down to the FiniteCoxeterGroups. I can do this if you agree with it (as I have superpowers).

FiniteCoxeterGroups pulls from CoxeterGroups right? So I should just be able to create the absolute_length function in there and it should take precedence correct? Or do I need to do anything difference since it's an element class within the object?

Yes, as you surmised, you just need to implement an absolute_length() method in FiniteCoxeterGroups.ElementMethods, then it will override the general case (without needing to check if it is a finite group or not, as that should already be done by choosing the category) in CoxeterGroups.ElementMethods (through Sage category black magic).

@thecaligarmo
Copy link
Contributor Author

Ok, these changes are done. Note that the reflections chain gives left multiplication instead of right. So I went ahead and gave an example of this to be extra clear how the multiplication is working.

@tscrim
Copy link
Collaborator

tscrim commented Jan 19, 2024

Thank you. Looks good. Just a few more small details.

  • My previous code-specific comments.
  • Using {{{}}} instead of $` in your docstring.
  • Copy the relevant doc from absolute_length() to the new method in FiniteCoxeterGroups and move the finite type tests over (the generic one is not being tested by those after all).

Apologies for the much longer review for a fairly simple thing to fix, but I appreciate how much of an improvement this change is contributing!

@thecaligarmo
Copy link
Contributor Author

thecaligarmo commented Jan 19, 2024

Thank you. Looks good. Just a few more small details.

  • My previous code-specific comments.

(Edit) I just noticed the changes. These should be fixed now as well.

  • Using {{{}}} instead of $` in your docstring.

Oops, it's been fixed.

  • Copy the relevant doc from absolute_length() to the new method in FiniteCoxeterGroups and move the finite type tests over (the generic one is not being tested by those after all).

And done =)

Apologies for the much longer review for a fairly simple thing to fix, but I appreciate how much of an improvement this change is contributing!

No! Don't apologize. This is much nicer now =D

thecaligarmo and others added 2 commits January 19, 2024 13:48
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. LGTM.

Copy link

Documentation preview for this PR (built with commit 231c475; changes) is ready! 🎉

@vbraun vbraun merged commit 0dbab3c into sagemath:develop Jan 22, 2024
22 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Absolute length returning wrong value in affine type
4 participants