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

schur functions construct elements with coefficients in the wrong base ring #33313

Closed
videlec opened this issue Feb 8, 2022 · 18 comments
Closed

Comments

@videlec
Copy link
Contributor

videlec commented Feb 8, 2022

sage: Sym = SymmetricFunctions(QQ['t'])
sage: Sym.schur()([[2], [1]]) / 2
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Input In [51], in <module>
----> 1 Sym.schur()([[Integer(2)], [Integer(1)]]) / Integer(2)

File /usr/lib/python3.10/site-packages/sage/modules/with_basis/indexed_element.pyx:896, in sage.modules.with_basis.indexed_element.IndexedFreeModuleElement.__truediv__ (build/cythonized/sage/modules/with_basis/indexed_element.c:9578)()

File /usr/lib/python3.10/site-packages/sage/categories/rings.py:1349, in Rings.ElementMethods._divide_if_possible(self, y)
   1347 q, r = self.quo_rem(y)
   1348 if r != 0:
-> 1349     raise ValueError("%s is not divisible by %s"%(self, y))
   1350 return q

ValueError: 1 is not divisible by 2

The underlying reason for this error is that coefficients are stored as integers

sage: Sym.schur()([[2], [1]]).coefficients()[0].parent()
Integer Ring

Original report https://groups.google.com/g/sage-support/c/vxgtsSwJxXE

Depends on #33267

CC: @tscrim

Component: combinatorics

Author: Travis Scrimshaw

Branch: 5ffa2f0

Reviewer: Vincent Delecroix

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

@videlec videlec added this to the sage-9.6 milestone Feb 8, 2022
@tscrim
Copy link
Collaborator

tscrim commented Feb 8, 2022

comment:1

Technically speaking, it shouldn’t matter that they are stored as integers as coercion should allow that to work. That being said, we can be more careful about how we build these things without a loss of optimization during the construction.

Without testing the code, I am a bit skeptical that there is not also a bug in the input because it should be a partition. I don’t quite understand why this is even constructing an element of the Schur functions, but I don’t have the code in front of me right now.

I will take a more detailed look tomorrow.

@tscrim
Copy link
Collaborator

tscrim commented Feb 9, 2022

comment:2

Ah, the input should be treated as a skew partition. This is where our problems comes from. Here is a more minimal example:

sage: s = SymmetricFunctions(GF(2)).s()
sage: s([[3,2,1],[2,1]])
s[1, 1, 1] + 2*s[2, 1] + s[3]
sage: s.skew_schur([[3,2,1],[2,1]])
s[1, 1, 1] + 2*s[2, 1] + s[3]

@tscrim
Copy link
Collaborator

tscrim commented Feb 9, 2022

Commit: 3fdf528

@tscrim
Copy link
Collaborator

tscrim commented Feb 9, 2022

@tscrim
Copy link
Collaborator

tscrim commented Feb 9, 2022

comment:3

Here is a fix, where this is exposing a deeper issue with not converting the coefficients correctly. So working over, say, GF(2) leads to coefficients with 2 appearing because they were integers. Now everything is coerced into the base ring and computations are done there.

I also did some optimizations for skew_by() and _apply_multi_module_morphism(), which came up when I was looking into this ticket.

sage: s = SymmetricFunctions(QQ).s()
sage: %timeit s[5,4,2,2,1].skew_by(s[3,2,2,1] + s[3,1])
366 µs ± 2.25 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

versus with #33267:

sage: %timeit s[5,4,2,2,1].skew_by(s[3,2,2,1] + s[3,1])
391 µs ± 3.62 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

versus 9.5:

sage: %timeit s[5,4,2,2,1].skew_by(s[3,2,2,1] + s[3,1])
404 µs ± 2.91 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

#33267 dependency is for a trivial conflict.


New commits:

6d1dbb2Speedup of sum_of_* methods by using dictionaries directly.
7229a19some details about shuffle of words and multizetas
ec60e55Merge branch 'u/chapoton/33102' in 9.5
7ea8aeafix a bug in multiple zeta values
c4415d7Merge branch 'u/chapoton/33257' of git://trac.sagemath.org/sage into public/performance/optimize_sum_of_in_cfm-33267
fd50bb4One additional optimization to multiple zetas.
33a7e77Update doc of sum_of_monomials() to include iterables.
bfe5203Fixing issues with coefficients not in the base ring for symmetric functions.
a3571eaOptimizing the skew_by() function.
3fdf528Micro-optimization for _apply_multi_module_morphism().

@tscrim
Copy link
Collaborator

tscrim commented Feb 9, 2022

Dependencies: #33267

@tscrim
Copy link
Collaborator

tscrim commented Feb 9, 2022

Author: Travis Scrimshaw

@videlec
Copy link
Contributor Author

videlec commented Feb 9, 2022

comment:4

In coerce_remove_zeros you do not apply a coercion but simply call the parent R. It would make more sense to either call it convert_remove_zeros or really use coercion val = R.coerce(D[index]).

@fchapoton
Copy link
Contributor

comment:5

pyflakes plugin is not happy about the remaining

         zero = s.zero()

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 10, 2022

Changed commit from 3fdf528 to 5ffa2f0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 10, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

5ffa2f0Incorporating comments from reviewers.

@tscrim
Copy link
Collaborator

tscrim commented Feb 10, 2022

comment:7

I have chosen to call is convert to be more flexible. I also removed the unused zero.

@tscrim
Copy link
Collaborator

tscrim commented Feb 12, 2022

comment:8

Patchbot is morally green (failure is due to #32773).

@videlec
Copy link
Contributor Author

videlec commented Feb 12, 2022

comment:9

Thanks for writing a fix.

@videlec
Copy link
Contributor Author

videlec commented Feb 12, 2022

Reviewer: Vincent Deleroix

@vbraun
Copy link
Member

vbraun commented Feb 20, 2022

Changed branch from public/combinat/skew_schur_coeff-33313 to 5ffa2f0

@mkoeppe
Copy link
Contributor

mkoeppe commented May 15, 2022

Changed commit from 5ffa2f0 to none

@mkoeppe
Copy link
Contributor

mkoeppe commented May 15, 2022

Changed reviewer from Vincent Deleroix to Vincent Delecroix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants