Opened 7 months ago

Closed 6 months ago

#27423 closed defect (fixed)

fix constructor of ConvexRationalPolyhedralCone

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-8.7
Component: geometry Keywords:
Cc: novoselt Merged in:
Authors: Vincent Delecroix Reviewers: Andrey Novoseltsev
Report Upstream: N/A Work issues:
Branch: d04b627 (Commits) Commit: d04b62728fefc25fa98742552ce9cad2f354ab6f
Dependencies: Stopgaps:

Description (last modified by novoselt)

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

Change History (11)

comment:1 Changed 7 months ago by vdelecroix

  • Description modified (diff)

comment:2 follow-up: Changed 7 months ago by novoselt

  • Description modified (diff)

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.

comment:3 in reply to: ↑ 2 Changed 7 months ago by vdelecroix

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.

comment:4 follow-up: Changed 7 months ago by vdelecroix

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.

comment:5 in reply to: ↑ 4 Changed 7 months ago by vdelecroix

Replying to vdelecroix:

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.

comment:6 Changed 7 months ago by novoselt

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.

comment:7 Changed 7 months ago by vdelecroix

  • Authors set to Vincent Delecroix
  • Branch set to u/vdelecroix/27423
  • Commit set to 5a5293c265a8b252e7f1691526540352ef0411ad
  • Status changed from new to needs_review

New commits:

5a5293cfix documentation of PPL parameter

comment:8 Changed 7 months ago by novoselt

  • Reviewers set to Andrey Novoseltsev

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

comment:9 Changed 7 months ago by git

  • Commit changed from 5a5293c265a8b252e7f1691526540352ef0411ad to d04b62728fefc25fa98742552ce9cad2f354ab6f

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

d04b627take -> make

comment:10 Changed 7 months ago by novoselt

  • Status changed from needs_review to positive_review

comment:11 Changed 6 months ago by vbraun

  • Branch changed from u/vdelecroix/27423 to d04b62728fefc25fa98742552ce9cad2f354ab6f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.