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 tscrim)

Implements the classical Hall algebra and the corresponding Hall polynomials.

Apply:

Attachments (1)

trac_15311-hall_algebras-ts.patch (34.2 KB) - added by tscrim 6 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 6 years ago by tscrim

  • Status changed from new to needs_review

comment:2 Changed 6 years ago by darij

  • Cc darij added

comment:3 follow-up: Changed 6 years ago by 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).

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?
  • I prevented the coercion to the symmetric functions from appearing unless q is invertible. Is this unnecessarily restrictive? (The map does involve division by q.)
  • Your scalar product functions had a q argument which seems pointless to me (the q is already an attribute of the parent, and I don't think you want to allow a different q in the scalar product -- if you do, you are doing it wrong). I removed it.

Nice work!

Version 0, edited 6 years ago by darij (next)

comment:4 Changed 6 years ago by darij

  • Description modified (diff)

comment:5 in reply to: ↑ 3 Changed 6 years ago by tscrim

  • 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 by q.)

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 (the q is already an attribute of the parent, and I don't think you want to allow a different q 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 darij

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 tscrim

  • 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 to hall_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 darij

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 tscrim

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 darij

  • 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. Or should I except another exception? (I don't really know what errors q**-1 can cause.)

for the patchbot:

apply trac_15311-hall_algebras-ts.patch​ trac_15311-rev-dg.patch

Last edited 6 years ago by darij (previous) (diff)

comment:11 Changed 6 years ago by tscrim

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 darij

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 tscrim

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 darij

Thank you! Positive review?

comment:15 Changed 6 years ago by tscrim

  • Reviewers set to Darij Grinberg
  • Status changed from needs_review to positive_review

Yep. Thanks Darij.

comment:16 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.13 to sage-pending

comment:17 Changed 6 years ago by tscrim

  • Milestone changed from sage-pending to sage-5.13

comment:18 Changed 6 years ago by jdemeyer

  • 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 tscrim

  • 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 jdemeyer

  • 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 tscrim

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 tscrim

comment:22 Changed 6 years ago by tscrim

  • 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 jdemeyer

Documentation works now...

comment:24 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.13.beta4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.