Opened 6 years ago
Closed 6 years ago
#21043 closed enhancement (fixed)
Use key instead of cmp in categories/modules_with_basis.py
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage7.3 
Component:  python3  Keywords:  
Cc:  tscrim, jmantysalo, chapoton, nthiery, darij  Merged in:  
Authors:  Frédéric Chapoton, Jeroen Demeyer  Reviewers:  Frédéric Chapoton, Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  38b29bb (Commits, GitHub, GitLab)  Commit:  38b29bb90ed4dc52999cc2e4cca2ca019ae80b77 
Dependencies:  Stopgaps: 
Description
Various functions dealing with support take a cmp
parameter. This should be changed to a key
parameter.
Change History (19)
comment:1 Changed 6 years ago by
comment:2 Changed 6 years ago by
 Cc nthiery darij added
This is likely to be a nontrivial change as we will probably also have to do work with the module morphisms as well. However, this will be good to get done at some point soon.
comment:3 Changed 6 years ago by
Shall we deprecate as usual ? It would be simpler to just replace by key, of course..
comment:4 followup: ↓ 5 Changed 6 years ago by
 Branch set to public/21043
 Commit set to 9f0a3acd7cfb036695ed968d115b29041e0305f5
 Status changed from new to needs_review
Not yet finished.
This triggers deprecations in
src/sage/algebras/iwahori_hecke_algebra.py
New commits:
9f0a3ac  trac 21043 first step of using key to sort basis of free modules

comment:5 in reply to: ↑ 4 ; followup: ↓ 7 Changed 6 years ago by
 Status changed from needs_review to needs_work
comment:6 Changed 6 years ago by
 Commit changed from 9f0a3acd7cfb036695ed968d115b29041e0305f5 to f5c91352bfa4b0e86f8aba1bc8e63b2e44f05939
Branch pushed to git repo; I updated commit sha1. New commits:
f5c9135  trac 21043 add deprecation in Iwahori Hecke

comment:7 in reply to: ↑ 5 Changed 6 years ago by
comment:8 Changed 6 years ago by
I see you try to avoid some boilerplate by separating the cmp_deprecation
function.
However, you could simplify a lot more (not tested):
def max_kwds(iterable, **kwds) if 'cmp' in kwds: deprecation(21043, "the 'cmp' keyword is deprecated, use 'key' instead") return max_cmp(iterable, kwds['cmp']) return max(iterable, **kwds) def min_kwds(iterable, **kwds) # analogous def leading_support(self, **kwds): return max_kwds(self.support(), **kwds)
which is simpler code with less boilerplate. If we ever remove the cmp
flag, we just need to replace max_kwds
by max
and min_kwds
by min
.
comment:9 followup: ↓ 11 Changed 6 years ago by
oh, well. Really needed ?
Both min_cmp
and max_cmp
are used only here, and you want to wrap them again ?
It would be better to change them directly. But I do not really see the point.
comment:10 Changed 6 years ago by
 Commit changed from f5c91352bfa4b0e86f8aba1bc8e63b2e44f05939 to a0fb84a08d867f4b71f82020115701ff1f8ee414
Branch pushed to git repo; I updated commit sha1. New commits:
a0fb84a  trac 21043 fixing another doctest

comment:11 in reply to: ↑ 9 Changed 6 years ago by
Replying to chapoton:
Both
min_cmp
andmax_cmp
are used only here, and you want to wrap them again ? It would be better to change them directly.
Fine for me!
But I do not really see the point.
Two points:
 Less code duplication.
 Simpler code (by avoiding duplication and using
**kwds
to remove theif key is not None
branch).
comment:12 Changed 6 years ago by
 Status changed from needs_work to needs_review
All tests pass. I will not be able to work on this for at least one or two weeks.
comment:13 Changed 6 years ago by
 Status changed from needs_review to needs_work
comment:14 Changed 6 years ago by
 Commit changed from a0fb84a08d867f4b71f82020115701ff1f8ee414 to 38b29bb90ed4dc52999cc2e4cca2ca019ae80b77
Branch pushed to git repo; I updated commit sha1. New commits:
38b29bb  Simplify code by using *args and **kwds

comment:15 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:16 Changed 6 years ago by
ok, thanks for doing the job. Your changes looks good to me and all tests pass.
You can set a positive review if you want.
comment:17 Changed 6 years ago by
 Reviewers set to Frédéric Chapoton, Jeroen Demeyer
comment:18 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:19 Changed 6 years ago by
 Branch changed from public/21043 to 38b29bb90ed4dc52999cc2e4cca2ca019ae80b77
 Resolution set to fixed
 Status changed from positive_review to closed
I do not plan to fix this now, I'm just creating the ticket in case anybody feels like working on this.