Opened 6 years ago
Closed 6 years ago
#15311 closed enhancement (fixed)
Implement the classical Hall algebra and polynomials
Reported by: | tscrim | Owned by: | tscrim |
---|---|---|---|
Priority: | major | Milestone: | sage-5.13 |
Component: | algebra | Keywords: | Hall algebra polynomials |
Cc: | amri, darij | Merged in: | sage-5.13.beta4 |
Authors: | Travis Scrimshaw | Reviewers: | Darij Grinberg |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #15305 | Stopgaps: |
Description (last modified by )
Implements the classical Hall algebra and the corresponding Hall polynomials.
Apply:
Attachments (1)
Change History (25)
comment:1 Changed 6 years ago by
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
- Cc darij added
comment:3 follow-up: ↓ 5 Changed 6 years ago by
comment:4 Changed 6 years ago by
- Description modified (diff)
comment:5 in reply to: ↑ 3 Changed 6 years ago by
- Dependencies set to #15305
Replying to darij:
Review patch uploaded. I've got only one issue I'd like you to fix (unless it's intention), and that's the fact that the Hall algebra doesn't coerce to the symmetric functions until you call the
HallAlgebraMonomials
basis (because only the latter basis activates the coercions).
I'll fix that in the next day or so.
Comments on my changes:
- Your patch contained some coercion fu in the coproduct method, where you took an element of the tensor square of one basis and coerced it into the tensor square of another. This doesn't work in the current version of Sage, not even with #10963 applied (therefore the failing doctests). I have replaced this by a manual implementation. Is this fixed on the combinat queue?
Ah, sorry I forgot the #15305 dependency which does this.
- I prevented the coercion to the symmetric functions from appearing unless
q
is invertible. Is this unnecessarily restrictive? (The map does involve division byq
.)
I believe it makes the scalar product agree with the HL scalar product... I saw something that this made the map "nice", but I don't think it was in Schiffmann.
- Your scalar product functions had a
q
argument which seems pointless to me (theq
is already an attribute of the parent, and I don't think you want to allow a differentq
in the scalar product -- if you do, you are doing it wrong). I removed it.
Fair enough.
Nice work!
Thanks, and thank you for reviewing this! I'll post a new version with your changes folded in once I've fixed the coercion issue.
See you soon,
Travis
comment:6 Changed 6 years ago by
BTW unless you married someone called Scirmshaw, there is one more misspelling of your name than what I caught :) (and many more in #15300)
comment:7 Changed 6 years ago by
- Description modified (diff)
I was trying out new spellings of my names, and then copy/paste fun! XP
So I've folded in your review patch and made the following changes:
- Fixed a bug so computations work for prime powers (I forgot to pass
self._q
tohall_polynomial()
). This uncovered another problem, that [univariate] Laurent polynomials don't divide like the should (see #11726), so I changed all of the tests to use the fraction field. - Fixed the coercion.
- Made
HallAlgebra
lazily imported. - Improved the doc and added some more doctests.
I didn't want to make this depend on #11726 since functionally it will work when Laurent polynomials do the division like they should (although it is horrendously slow with the current #11726 patch).
For patchbot:
Apply: trac_15311-hall_algebra-ts.patch
comment:8 Changed 6 years ago by
Did you actually do these changes? Because my edits aren't there and I still don't see HallAlgebra? lazily imported. Looks like phases hit you again? (BTW, are there any good rules on when to import lazily vs. regularly, and how to import in general? Like, is it better to import ZZ
from sage.rings.all, or rather define ZZ = IntegerRing()
after importing IntegerRing
from sage.rings.integer_ring
?)
comment:9 Changed 6 years ago by
Ah sorry, I upgraded to beta2
but uploaded from the old one.
Since this is a niche thing IMO (wrt the rest of Sage), it's worthwhile it lazily import it to help keep startup time down. As far as the ZZ
import, it just makes things easier for me to remember; I don't think there's an explicit difference other than having extra unneeded (in a loose sense of the word) variables running around.
For patchbot:
Apply: trac_15311-hall_algebra-ts.patch
comment:10 Changed 6 years ago by
- Description modified (diff)
Sorry, I forgot about specializations. Here is a fix for another thing I failed to notice: the q=0 case. If you like it, please set to positive_review.
for the patchbot:
apply trac_15311-hall_algebras-ts.patch trac_15311-rev-dg.patch
comment:11 Changed 6 years ago by
I believe the correct syntax is except (ZeroDivisionErorr, NotImplementedError):
. Once this change is made, then you can set it to positive review. Thanks Darij.
comment:12 Changed 6 years ago by
I've taken a different solution, seeing that I didn't catch ValueError
which too appears when trying to invert some things. I actually think this is one of the cases where an unqualified except
is legit. If you're OK with this, please set to positive_review.
comment:13 Changed 6 years ago by
You should use except StandardError:
, that way it doesn't catch things like keyboard interrupts and other more serious system errors.
comment:14 Changed 6 years ago by
Thank you! Positive review?
comment:15 Changed 6 years ago by
- Reviewers set to Darij Grinberg
- Status changed from needs_review to positive_review
Yep. Thanks Darij.
comment:16 Changed 6 years ago by
- Milestone changed from sage-5.13 to sage-pending
comment:17 Changed 6 years ago by
- Milestone changed from sage-pending to sage-5.13
comment:18 Changed 6 years ago by
- Status changed from positive_review to needs_work
The PDF doesn't build:
! Emergency stop. <to be read again> $ l.5618 unity of this algebra is $I_\empty$ . ! ==> Fatal error occurred, no output PDF file produced!
comment:19 Changed 6 years ago by
- Description modified (diff)
- Status changed from needs_work to positive_review
Fixed (and I folded in the last review patch).
Output written on algebras.pdf (170 pages, 799758 bytes). Transcript written on algebras.log. Build finished. The built documents can be found in /home/travis/sage/src/doc/output/pdf/en/reference/algebras
For patchbot:
Apply: trac_15311-hall_algebras-ts.patch
comment:20 Changed 6 years ago by
- Status changed from positive_review to needs_work
The docbuilder reports:
dochtml.log:[algebras ] None:33: WARNING: citation not found: Schiffmann
comment:21 Changed 6 years ago by
Jeroen, I though references were global? The reference is defined in sage/combinat/hall_polynomial.py
(lines 129-130):
.. [Schiffmann] Oliver Schiffmann. *Lectures on Hall algebras*. :arxiv:`0611617v2`.
I'm running a full docbuild right now to check.
Changed 6 years ago by
comment:22 Changed 6 years ago by
- Status changed from needs_work to positive_review
...Or the hall_polynomial.py
file was not included in the documentation. Sorry. Fixed.
For patchbot:
Apply: trac_15311-hall_algebras-ts.patch
comment:23 Changed 6 years ago by
Documentation works now...
comment:24 Changed 6 years ago by
- Merged in set to sage-5.13.beta4
- Resolution set to fixed
- Status changed from positive_review to closed
Review patch uploaded. I've got only one issue I'd like you to fix (unless it's intention), and that's the fact that the Hall algebra doesn't coerce to the symmetric functions until you call the
HallAlgebraMonomials
basis (because only the latter basis activates the coercions).Comments on my changes:
q
is invertible. Is this unnecessarily restrictive? (The map does involve division byq
.)q
argument which seems pointless to me (theq
is already an attribute of the parent, and I don't think you want to allow a differentq
in the scalar product -- if you do, you are doing it wrong). I removed it.Nice work!
for the patchbot:
apply trac_15311-hall_algebras-ts.patch trac_15311-rev-dg.patch