Opened 7 years ago

Closed 7 years ago

#16410 closed enhancement (fixed)

Make combinatorial_map a no-op by default.

Reported by: nthiery Owned by:
Priority: major Milestone: sage-6.3
Component: combinatorics Keywords: combinatorial_map, findstat
Cc: sage-combinat, ncohen, vdelecroix, stump, VivianePons Merged in:
Authors: Nicolas M. Thiéry Reviewers: Christian Stump
Report Upstream: N/A Work issues:
Branch: 0cc67e9 (Commits, GitHub, GitLab) Commit: 0cc67e94a1fa27e955b2820a7b0f729e3f8275f1
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (18)

comment:1 Changed 7 years ago by nthiery

  • Branch set to u/nthiery/make_combinatorial_map_a_no_op_by_default_

comment:2 Changed 7 years ago by vdelecroix

  • Commit set to 0cc67e94a1fa27e955b2820a7b0f729e3f8275f1

Hum: #16408


New commits:

0cc67e916410: Make combinatorial_map a no-op by default.

comment:3 follow-up: Changed 7 years ago by vdelecroix

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

comment:4 follow-up: Changed 7 years ago by 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.

comment:5 in reply to: ↑ 4 Changed 7 years ago by nthiery

Replying to 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.

comment:6 in reply to: ↑ 3 ; follow-up: Changed 7 years ago by nthiery

Replying to vdelecroix:

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=True` before returning f, and have combinatorial_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

comment:7 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by vdelecroix

Replying to nthiery:

Replying to vdelecroix:

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

comment:8 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by stumpc5

Replying to vdelecroix:

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

comment:9 Changed 7 years ago by VivianePons

  • Cc VivianePons added

comment:10 Changed 7 years ago by VivianePons

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.

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

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

comment:12 Changed 7 years ago by nthiery

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.

comment:13 Changed 7 years ago by nthiery

For the record: all long tests passed.

comment:14 follow-up: Changed 7 years ago by tscrim

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

Version 0, edited 7 years ago by tscrim (next)

comment:15 in reply to: ↑ 14 Changed 7 years ago by nthiery

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.

comment:16 Changed 7 years ago by stumpc5

  • Reviewers set to Christian Stump
  • Status changed from new to needs_review

comment:17 Changed 7 years ago by stumpc5

  • Status changed from needs_review to positive_review

comment:18 Changed 7 years ago by vbraun

  • Branch changed from u/nthiery/make_combinatorial_map_a_no_op_by_default_ to 0cc67e94a1fa27e955b2820a7b0f729e3f8275f1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.