Make combinatorial_map a no-op by default.

[nthiery]

From the discussions of (TODO: add refs to the threads in sage-devel),
there is a consensus that, at this point, the combinatorial_map
decorator should by default return the decorated method as is, in
order to have no impact on speed. On the other hand, projects built on
top of Sage, like findstat are welcome to customize locally this hook to instrument
the Sage code and exploit the semantic information provided by this
decorator.

This ticket therefore:

• Defines combinatorial_map as a no-op
• Discuss the purpose of this decorator
• Uses the previous implementation as an example of how to customize
  combinatorial_map

Note: this change is slightly backward incompatible since combinatorial_maps_in_class is not functional any more by default.

CC: @sagetrac-sage-combinat @nathanncohen @videlec @stumpc5 @VivianePons

Component: combinatorics

Keywords: combinatorial_map, findstat

Author: Nicolas M. Thiéry

Branch: 0cc67e9

Reviewer: Christian Stump

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

[nthiery]

Branch: u/nthiery/make_combinatorial_map_a_no_op_by_default_

[videlec]

comment:2

Hum: #16408

---

New commits:

0cc67e9

16410: Make combinatorial_map a no-op by default.

[videlec]

Commit: 0cc67e9

[videlec]

comment:3

Hi Nicolas,

I do not like the fact that in order to use combinatorial maps we need to modify the source code. The decorator is executed when the source code is loaded, hence on startup. Turning it on inside a console would just be a nightmare. This is a killer strategy: you simply removed combinatorial maps from being usable.

On the other hand, this ticket can be reviewed quickly compared to #16408 (which needs some design discussions). We can let this one go and then start discussing #16408 but I think we need more opinions on this.

Thanks
Vincent

[sagetrac-tmonteil]

comment:4

The description says : "[...] there is a consensus that, at this point, the combinatorial_map decorator should by default return [...]"

For the record, there is no consensus that there should be any decorator for this.

[nthiery]

comment:5

Replying to @sagetrac-tmonteil:

The description says : "[...] there is a consensus that, at this point, the combinatorial_map decorator should by default return [...]"

For the record, there is no consensus that there should be any decorator for this.

As far as I remember, even Nathann agreed that he could see the point of having such a decorator. In any cases, this is orthongonal to this ticket. So let's focus and act.

[nthiery]

comment:6

Replying to @videlec:

I do not like the fact that in order to use combinatorial maps we need to modify the source code. The decorator is executed when the source code is loaded, hence on startup. Turning it on inside a console would just be a nightmare. This is a killer strategy: you simply removed combinatorial maps from being usable.

I agree, it's unusable from a plain Sage. But it remains usable by
findstat and friends which, as far as I know, are the only serious use
cases so far, while removing the primary gripe against
combinatorial_map.

Note: if we really care about keeping combinatorial_maps_in_class in
working state because some people might have been using, we could, as
a temporary trick, have the decorator do
'f._is_combinatorial_map=Truebefore returning f, and havecombinatorial_maps_in_class` check for this attribute.

On the other hand, this ticket can be reviewed quickly compared to #16408 (which needs some design discussions).

Precisely :-)

We can let this one go and then start discussing #16408 but I think we need more opinions on this.

By the way: how bad is it to merge this on into #16408? I am sorry to
be getting in your way ...

Cheers,
Nicolas

[videlec]

comment:7

Replying to @nthiery:

Replying to @videlec:

We can let this one go and then start discussing #16408 but I think we need more opinions on this.

By the way: how bad is it to merge this on into #16408? I am sorry to
be getting in your way ...

It would be nothing to adapt #16408 (what I have done is only a draft experimentation). So it is not an argument against this ticket. But, I do not see the emergency of getting rid of combinatorial_map (we are in early beta so we have time to think for the next stable).

Vincent

[stumpc5]

comment:8

Replying to @videlec:

But, I do not see the emergency of getting rid of combinatorial_map

There is no emergency - but I do think at this point it would be better to actually completely remove combinatorial_map. For us, it was never important to have the decorator in public Sage, and I regret to have ever sent it since it turned out to be just a big waste of energy. (But when I wrote that code, people agreed that they want to have it, and I have seen at least Anne S and Martin R using it).

Not having it there anymore opens more possibilities to think of what the right way to proceed is (then using #16408).

Christian

[VivianePons]

comment:10

This ticket seems a good solution for the time being. It leaves here the semantic information that have been added by many users and fixes the issues of wrapping the methods with the decorator.

I think we all agree this is not a long term solution but it will give us time to do things the right way on Vincent's ticket.

[nthiery]

comment:11

Replying to @stumpc5:

There is no emergency - but I do think at this point it would be
better to actually completely remove combinatorial_map.
For us, it was never important to have the decorator in public
Sage, and I regret to have ever sent it since it turned out to be
just a big waste of energy. (But when I wrote that code, people
agreed that they want to have it, and I have seen at least Anne S
and Martin R using it).

I see your point. From a social point we, as a community, screwed up:
finding an acceptable common ground for everyone should not have taken
more than a good one-hour brainstorm, and a couple of hours of
coding/reviewing. We had plenty of occasions for this! I am frustrated
myself about all the energy and hurt feelings we wasted there.

This does not necessarily mean, though, that this was not the right
thing to do technically speaking.

Anyway, whether combinatorial_map is the right long term approach or
not is orthogonal to this ticket.

Let's move on, focus on the technical solutions, and act together;
that's the best way to heal the community.

Cheers!
Nicolas

[nthiery]

comment:12

Sounds like everybody agrees that this is a reasonable short term solution. Can someone review the details, and set a positive review? I am running all long tests now.

[nthiery]

comment:13

For the record: all long tests passed.

[tscrim]

comment:14

Note to self [and other FindStat devs] to make sure to undo this once this is merged (and to cc myself on this).

[nthiery]

comment:15

Replying to @tscrim:

Note to self [and other FindStat devs] to make sure to undo this once this is merged (and to cc myself on this).

Yup. Note that you just have a single line to change; not the full ticket.

[stumpc5]

Reviewer: Christian Stump

[vbraun]

Changed branch from u/nthiery/make_combinatorial_map_a_no_op_by_default_ to 0cc67e9

[fchapoton]

Changed commit from 0cc67e9 to none