Opened 8 years ago

Closed 8 years ago

#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 Merged in: sage-4.3.2.alpha0
Authors: Jason Bandlow Reviewers: Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jbandlow)

I'm including a patch, but will rebase it against 4.3.1 once it is released.

Attachments (3)

swap_term_and_monomial-jb.patch (53.9 KB) - added by jbandlow 8 years ago.
trac_7938_swap_term_and_monomial-jb.patch (53.7 KB) - added by jbandlow 8 years ago.
trac_7938_swap_term_and_monomial-jb.2.patch (53.7 KB) - added by nthiery 8 years ago.
Rebased for 4.3.1

Download all attachments as: .zip

Change History (13)

Changed 8 years ago by jbandlow

comment:1 Changed 8 years ago by jbandlow

  • Component changed from algebra to categories
  • Description modified (diff)
  • Status changed from new to needs_work

comment:2 follow-up: Changed 8 years ago by nthiery

  • Cc sage-combinat added; nthiery removed
  • Milestone changed from sage-4.3.1 to sage-4.3.2
  • Reviewers set to Nicolas M. Thiéry

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 8 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.

Changed 8 years ago by jbandlow

comment:4 follow-up: Changed 8 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: Changed 8 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,

comment:6 follow-up: Changed 8 years ago by nthiery

It does need a rebase w.r.t. #7729 (iwahori hecke algebra) whose file was renamed.

Please update the queue accordingly (including the #7729 patch).

comment:7 in reply to: ↑ 6 Changed 8 years ago by nthiery

Replying to nthiery:

It does need a rebase w.r.t. #7729 (iwahori hecke algebra) whose file was renamed.

Please update the queue accordingly (including the #7729 patch).

Done!

Changed 8 years ago by nthiery

Rebased for 4.3.1

comment:8 in reply to: ↑ 5 Changed 8 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:9 Changed 8 years ago by nthiery

  • Status changed from needs_review to positive_review

comment:10 Changed 8 years ago by mvngu

  • Authors changed from jbandlow to Jason Bandlow
  • Merged in set to sage-4.3.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.