Replace generator_cmp by generator_key and generator_reverse

[a-andre]

Deprecate the print option generator_cmp in favour of sorting_key and sorting_reverse.

This is part of #16536.

CC: @sagetrac-sage-combinat @nthiery @tscrim

Component: python3

Keywords: days74

Author: André Apitzsch

Branch/Commit: 42658f1

Reviewer: Travis Scrimshaw, Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/17229

[a-andre]

Commit: f7edf1e

[a-andre]

Branch: u/aapitzsch/ticket/17229

[videlec]

comment:2

Hello,

1. Could you motivate a bit more this change. I do not find simpler to use a key function instead of a cmp function.

2. It does not apply on the most recent version of Sage.

3. I found the docstring confusing

def indices_key(self):
    r""""
    A comparison function on sets which gives a linear extension
    of the inclusion order

This is not a comparison function (which would have pairs as input)!

More to come, when you answer 1

Vincent

[a-andre]

comment:3

Replying to @videlec:

1. Could you motivate a bit more this change. I do not find simpler to use a key function instead of a cmp function.

In Python 3 the cmp keyword has been removed in favor of key and reverse arguments. To reduce the overhead for a user to provide cmp and key function (e.g. one for sage's print_options() and the other for python's sort()) I suggest to harmonize the parameters.

[sagetrac-git]

Changed commit from f7edf1e to 6beb1ed

[sagetrac-git]

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

c69512f	Merge remote-tracking branch 'origin/develop' into py3_cmp_param
6beb1ed	fix docstring

[fchapoton]

Changed commit from 6beb1ed to dc29689

[fchapoton]

comment:7

rebased on 7.2.rc2

---

New commits:

dc29689

Merge branch 'u/aapitzsch/ticket/17229' into 7.2.rc2

[fchapoton]

Changed branch from u/aapitzsch/ticket/17229 to public/17229

[sagetrac-git]

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

9b6943c	Merge branch 'public/17229' into 7.2
7771ca8	trac 17229 preparing orlik-solomon to use key sort

[sagetrac-git]

Changed commit from dc29689 to 7771ca8

[sagetrac-git]

Changed commit from 7771ca8 to 4b45ecd

[sagetrac-git]

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

4b45ecd

trac 17229 changes from cmp to key in example of filtered algebras

[sagetrac-git]

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

971d3ad

trac 17229 new style doctest continuation

[sagetrac-git]

Changed commit from 4b45ecd to 971d3ad

[nthiery]

comment:13

I just went through the diff, and it sounds reasonable (haven't checked the details though).

Do we want to use the occasion to find better names for the options? generator_key and generator_reverse can be confusing (e.g. in an algebra one may think that this is about the algebra generators; also one may believe that this is about recovering the index of a generator in its family). Given that the sorting function is called sorted_items in CombinatorialFreeModule and IndexedFreeMonoid, I would lean toward something like sort_key and sort_reverse.

Tiny detail:

• in filtered_algebras_with_basis.py, sort_key would better be a method, with doctest, as in orlik_solomon.py

[sagetrac-git]

Changed commit from 971d3ad to 916f4bf

[sagetrac-git]

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

a7df3ea	Merge branch 'public/17229' into 7.3.b0
916f4bf	trac 17229 _sort_key as an hidden method

[sagetrac-git]

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

4b7d6ad	Merge branch 'public/17229' of trac.sagemath.org:sage into public/17229
7b6f82a	Changing generator_key/reverse to sorting_key/reverse.

[sagetrac-git]

Changed commit from 916f4bf to 7b6f82a

[tscrim]

comment:17

After a discussion with Nicolas, we decided to use sorting_key and sorting_reverse to avoid any potential ambiguity.

[tscrim]

Reviewer: Travis Scrimshaw

[fchapoton]

comment:18

There should be a comma or a dot somewhere in the sentence

Option monomial_cmp is deprecated use sorting_key and sorting_reverse instead.

[sagetrac-git]

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

db88b8c	Merge branch 'public/17229' into 7.3.b2
3b81594	trac 17229 adding a comma

[sagetrac-git]

Changed commit from 7b6f82a to 3b81594

[fchapoton]

comment:20

ok, looks almost good to me.

in the example

sorted(A.indices(), key=cmp_to_key(A.indices_cmp))

one should use instead the new indices_key

Maybe also we should avoid using cmp_to_key as much as possible.

[fchapoton]

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Frédéric Chapoton

[sagetrac-git]

Changed commit from 3b81594 to 42658f1

[sagetrac-git]

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

42658f1

Deprecating indices_cmp.

[tscrim]

comment:22

I decided to deprecate that function since it will be going away. However, because of how module morphism currently works, I had to write a small lambda function (which will vanish once we change this behavior in module_morphism).

[tscrim]

Changed keywords from none to days74

[fchapoton]

comment:24

ok, looks good to me

[tscrim]

comment:25

Thank you!

[vbraun]

Changed branch from public/17229 to 42658f1