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)
Change History (18)
Changed 8 years ago by
comment:1 Changed 8 years ago by
- Cc sage-combinat added
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
comment:3 Changed 7 years ago by
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
Changed 7 years ago by
Changed 7 years ago by
Changed 7 years ago by
comment:4 follow-up: ↓ 5 Changed 7 years ago by
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
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
- 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.
comment:7 Changed 7 years ago by
- 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.
comment:8 follow-up: ↓ 10 Changed 7 years ago by
- 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
- Reviewers set to nthiery
comment:10 in reply to: ↑ 8 Changed 7 years ago by
- 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
invertmethod:- 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
- 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
Depends on #7729.