Opened 8 years ago

Closed 7 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:

Description

Title says it all

Attachments (7)

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

Download all attachments as: .zip

Change History (18)

Changed 8 years ago by jbandlow

comment:1 Changed 8 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 7 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 7 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 7 years ago by jbandlow

Changed 7 years ago by jbandlow

Changed 7 years ago by jbandlow

comment:4 follow-up: Changed 7 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 7 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 7 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 7 years ago by nthiery

Apply only this one (includes the rebasing by Florent)

comment:7 Changed 7 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 7 years ago by jbandlow

Apply only this patch.

comment:8 follow-up: Changed 7 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 7 years ago by jbandlow

  • Reviewers set to nthiery

comment:10 in reply to: ↑ 8 Changed 7 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 7 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.