Opened 12 years ago

Closed 12 years ago

#7914 closed enhancement (fixed)

Implementation of triangular morphisms for modules with basis

Reported by: jbandlow Owned by: jbandlow
Priority: major Milestone: sage-4.4
Component: categories Keywords:
Cc: sage-combinat, adrien.boussicault@… Merged in: sage-4.4.alpha0
Authors: Jason Bandlow Reviewers: Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Title says it all

Attachments (7)

trac_7914_triangular-morphisms-jb.patch (18.3 KB) - added by jbandlow 12 years ago.
trac_7914_triangular_morphisms-review-nt.patch (34.2 KB) - added by nthiery 12 years ago.
trac_7914_triangular_morphisms-jb.patch (25.7 KB) - added by jbandlow 12 years ago.
trac_7914_triangular_morphisms-doc-jb.patch (6.5 KB) - added by jbandlow 12 years ago.
trac_7914_folded.patch (31.3 KB) - added by jbandlow 12 years ago.
trac_7914_folded_rebased.patch (31.7 KB) - added by nthiery 12 years ago.
Apply only this one (includes the rebasing by Florent)
trac_7914_rebased_fixed.patch (31.7 KB) - added by jbandlow 12 years ago.
Apply only this patch.

Download all attachments as: .zip

Change History (18)

Changed 12 years ago by jbandlow

comment:1 Changed 12 years ago by jbandlow

  • Authors set to jbandlow
  • Cc sage-combinat added
  • Status changed from new to needs_review

Depends on #7729.

comment:2 Changed 12 years ago by jbandlow

Apply only the second patch (sorry about the first one). This was tested on top of sage-4.3.1 + #7976, #7921, #7938, #8028, #8001, #5524.

comment:3 Changed 12 years ago by jhpalmieri

Title says it all

Actually, it doesn't: what's a triangular morphism? A definition (or at the very least a reference) should be in the documentation somewhere.

Changed 12 years ago by jbandlow

Changed 12 years ago by jbandlow

Changed 12 years ago by jbandlow

comment:4 follow-up: Changed 12 years ago by jbandlow

Nicolas, I added the patch trac_7914_triangular_morphisms-doc-jb.patch on top of your reviewer patch to fix some documentation issues (replacing 'maximal' with 'minimal' and that sort of thing). Since the patches got out of order on the ticket, I created trac_7914_folded.patch. This is all that needs to be applied. It applies cleanly to sage 4.3.2. and passes tests. I approve of Nicolas' reviewer changes (which are part of the folded patch), and believe this is ready for merging. Note: John Palmieri's comments were useful, and have been addressed in the documentation.

comment:5 in reply to: ↑ 4 Changed 12 years ago by hivert

I've rebased the patch on the sage-combinat queue. since there was a conflict with #8492. Please check are re-upload.

comment:6 Changed 12 years ago by nthiery

  • Status changed from needs_review to positive_review

Hi!

Sorry it took me so long to get on this one. Your changes are good. All test pass on sag.math. Positive review!

And thanks so much for your work on this feature.

Changed 12 years ago by nthiery

Apply only this one (includes the rebasing by Florent)

comment:7 Changed 12 years ago by jbandlow

  • Status changed from positive_review to needs_work

I found a small but critical bug when actually trying to use this. The invert method incorrectly defines 'domain' and 'codomain'. Fix coming shortly.

Changed 12 years ago by jbandlow

Apply only this patch.

comment:8 follow-up: Changed 12 years ago by jbandlow

  • Status changed from needs_work to needs_review

Fixed. The only change between the new patch and the previous one is line 1076 of modules_with_basis.py, in the invert method:

-                 domain = self.domain(),               codomain = self.codomain(), 
+                 domain = self.codomain(),             codomain = self.domain(), 

comment:9 Changed 12 years ago by jbandlow

  • Reviewers set to nthiery

comment:10 in reply to: ↑ 8 Changed 12 years ago by nthiery

  • Authors changed from jbandlow to Jason Bandlow
  • Cc adrien.boussicault@… added
  • Reviewers changed from nthiery to Nicolas M. Thiéry
  • Status changed from needs_review to positive_review

Replying to jbandlow:

Fixed. The only change between the new patch and the previous one is line 1076 of modules_with_basis.py, in the invert method:

-                 domain = self.domain(),               codomain = self.codomain(), 
+                 domain = self.codomain(),             codomain = self.domain(), 

Yes, Adrien had noticed this, and was about to fix it in the Sage-Combinat queue. Since this patch is not yet in Sage, I agree that it's best to include the fix right away in it. Positive review!

comment:11 Changed 12 years ago by jhpalmieri

  • Merged in set to sage-4.4.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

Merged in 4.4.alpha0:

  • trac_7914_rebased_fixed.patch
Note: See TracTickets for help on using tickets.