Ticket #7938 (closed enhancement: fixed)
'term' and 'monomial' are inconsistently used in some Category and combinat code
| Reported by: | jbandlow | Owned by: | AlexGhitza |
|---|---|---|---|
| Priority: | major | Milestone: | sage-4.3.2 |
| Component: | categories | Keywords: | |
| Cc: | sage-combinat | Work issues: | |
| Report Upstream: | N/A | Reviewers: | Nicolas M. Thiéry |
| Authors: | Jason Bandlow | Merged in: | sage-4.3.2.alpha0 |
| Dependencies: | Stopgaps: |
Description (last modified by jbandlow) (diff)
I'm including a patch, but will rebase it against 4.3.1 once it is released.
Attachments
-
swap_term_and_monomial-jb.patch
(53.9 KB) -
added by jbandlow 3 years ago.
-
trac_7938_swap_term_and_monomial-jb.patch
(53.7 KB) -
added by jbandlow 3 years ago.
-
trac_7938_swap_term_and_monomial-jb.2.patch
(53.7 KB) -
added by nthiery 3 years ago.
- Rebased for 4.3.1
Change History
comment:1 Changed 3 years ago by jbandlow
- Status changed from new to needs_work
- Component changed from algebra to categories
- Description modified (diff)
comment:2 follow-up: ↓ 3 Changed 3 years ago by nthiery
- Cc sage-combinat added; nthiery removed
- Reviewers set to Nicolas M. Thiéry
- Milestone changed from sage-4.3.1 to sage-4.3.2
This change has been discussed and approved on sage-devel and sage-combinat-devel. I went through the current patch and it looks good. This is a conditional positive review, pending a rebase on 4.3.1 (if at all needed), a recheck of all tests passing, and two little details:
- Renaming the patch to trac_7938_swap_term_and_monomial-jb.patch
- Adding a short description on top of it; say something like: "#7938: swap term and monomial in category and combinat code for consistency with the rest of sage"
Thanks Jason!
comment:3 in reply to: ↑ 2 Changed 3 years ago by nthiery
Replying to nthiery: Oops, I forgot the following:
- Removing the spurious change to sage/combinat/crystals/affine.py
- Adding a default value (the one of the coeff ring) for coeff in the new self.term, to make it backward compatible
- Only if easy, make the new monomial accept a second optional coeff argument, to make it backward compatible. This could be a bit tricky, since self.monomial is a Map. It is also not as important as for term, since I expect the later to have been used far more more extensively than the former.
comment:4 follow-up: ↓ 5 Changed 3 years ago by jbandlow
The patch called 'trac_7938_...' is all that should be applied. In response to Nicolas' comments:
- Rename patch: done
- Add description: done
- Remove spurious change to affine.py: done, but see #7978
- Add default value for coeff in self.term: done
- Add optional coeff argument to monomial: Not done yet. I'll look more (but maybe not much more) at this later.
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 8 Changed 3 years ago by nthiery
Replying to jbandlow:
...
Thanks much!
- Add description: done
Sorry for bothering you again. Please also remove the [mq] line, and put the rest on one line (hg treats the first line specifically).
Cheers,
Changed 3 years ago by nthiery
-
attachment
trac_7938_swap_term_and_monomial-jb.2.patch
added
Rebased for 4.3.1
comment:8 in reply to: ↑ 5 Changed 3 years ago by nthiery
- Status changed from needs_work to needs_review
Sorry for bothering you again. Please also remove the [mq] line, and put the rest on one line (hg treats the first line specifically).
Done. Patch ready for merging into Sage.
comment:10 Changed 3 years ago by mvngu
- Status changed from positive_review to closed
- Resolution set to fixed
- Merged in set to sage-4.3.2.alpha0
- Authors changed from jbandlow to Jason Bandlow
