fix constructor of ConvexRationalPolyhedralCone

[videlec]

The class ConvexRationalPolyhedralCone (sage/geometry/cone.py) has an "advanced" constructor parameter PPL that can be set to a ppl.polyhedron.C_Polyhedron. However, before #23024, this attribute used to be set immutable in the constructor and this is not the case anymore (since pplpy does not support the mutability/immutability flag).

This ticket stands to fix this possibly problematic behavior. Some options:

1. reintroduce mutability/immutability in pplpy
2. make a copy of the polyhedron (this definitely should not be done - the reason for this parameter is to avoid any work when PPL representation is already constructed)
3. keep it the way it is but fix the documentation

CC: @novoselt

Component: geometry

Author: Vincent Delecroix

Branch/Commit: d04b627

Reviewer: Andrey Novoseltsev

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

[novoselt]

comment:2

How feasible is the first option? I imagine what has to be done is adding a flag and then making sure that it is checked in all functions (current and future) that may change the polyhedron.

[videlec]

comment:3

Replying to @novoselt:

How feasible is the first option? I imagine what has to be done is adding a flag and then making sure that it is checked in all functions (current and future) that may change the polyhedron.

PPL (the C++ library) does not implement any mutability flag. This is why it has disappeared in the Python interface pplpy. A mutability flag is definitely error prone since you can still modify the polyhedron via C++ access to thisptr. Unless you have a serious use case for this immutability flag I am not inclined to reintroduce it.

[videlec]

comment:4

What is the problem with option 2? Copying the whole PPL object (together with all its data) is doable and certainly not expensive unless it is gigantic.

[videlec]

comment:5

Replying to @videlec:

What is the problem with option 2? Copying the whole PPL object (together with all its data) is doable and certainly not expensive unless it is gigantic.

Indeed, PPL has copy constructor which I believe does the right thing.

[novoselt]

comment:6

What if it is not gigantic, but there are 400 millions of them? What's the point to copy it if you specifically produced a PPL object to feed into constructor? Given that it is unlikely to be used "by accident", I think it should be kept as is, but the documentation should clearly say that it will be stored in a private attribute and will be assumed not to change.

[videlec]

New commits:

5a5293c

fix documentation of PPL parameter

[videlec]

Commit: 5a5293c

[videlec]

Author: Vincent Delecroix

[videlec]

Branch: u/vdelecroix/27423

[novoselt]

Reviewer: Andrey Novoseltsev

[novoselt]

comment:8

Should it be "make a copy" rather than "take a copy"?

[sagetrac-git]

Changed commit from 5a5293c to d04b627

[sagetrac-git]

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

d04b627

take -> make

[vbraun]

Changed branch from u/vdelecroix/27423 to d04b627