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: sage-7.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:

Status badges

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 jdemeyer

I do not plan to fix this now, I'm just creating the ticket in case anybody feels like working on this.

comment:2 Changed 6 years ago by tscrim

  • Cc nthiery darij added

This is likely to be a non-trivial 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 chapoton

Shall we deprecate as usual ? It would be simpler to just replace by key, of course..

comment:4 follow-up: Changed 6 years ago by chapoton

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

9f0a3actrac 21043 first step of using key to sort basis of free modules

comment:5 in reply to: ↑ 4 ; follow-up: Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to chapoton:

Not yet finished.

Then why needs_review?

comment:6 Changed 6 years ago by git

  • Commit changed from 9f0a3acd7cfb036695ed968d115b29041e0305f5 to f5c91352bfa4b0e86f8aba1bc8e63b2e44f05939

Branch pushed to git repo; I updated commit sha1. New commits:

f5c9135trac 21043 add deprecation in Iwahori Hecke

comment:7 in reply to: ↑ 5 Changed 6 years ago by chapoton

Replying to jdemeyer:

Replying to chapoton:

Not yet finished.

Then why needs_review?

This was just as a step to "needs_work". Sorry for not being as fast as light.

comment:8 Changed 6 years ago by jdemeyer

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.

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:9 follow-up: Changed 6 years ago by chapoton

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 git

  • Commit changed from f5c91352bfa4b0e86f8aba1bc8e63b2e44f05939 to a0fb84a08d867f4b71f82020115701ff1f8ee414

Branch pushed to git repo; I updated commit sha1. New commits:

a0fb84atrac 21043 fixing another doctest

comment:11 in reply to: ↑ 9 Changed 6 years ago by jdemeyer

Replying to chapoton:

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.

Fine for me!

But I do not really see the point.

Two points:

  1. Less code duplication.
  1. Simpler code (by avoiding duplication and using **kwds to remove the if key is not None branch).

comment:12 Changed 6 years ago by chapoton

  • 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 jdemeyer

  • Authors set to Frédéric Chapoton, Jeroen Demeyer
  • Status changed from needs_review to needs_work

comment:14 Changed 6 years ago by git

  • Commit changed from a0fb84a08d867f4b71f82020115701ff1f8ee414 to 38b29bb90ed4dc52999cc2e4cca2ca019ae80b77

Branch pushed to git repo; I updated commit sha1. New commits:

38b29bbSimplify code by using *args and **kwds

comment:15 Changed 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:16 Changed 6 years ago by chapoton

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 chapoton

  • Reviewers set to Frédéric Chapoton, Jeroen Demeyer

comment:18 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:19 Changed 6 years ago by vbraun

  • Branch changed from public/21043 to 38b29bb90ed4dc52999cc2e4cca2ca019ae80b77
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.