Opened 4 months ago

Closed 3 months ago

#29326 closed enhancement (fixed)

Improvements for projection into affine hull

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.1
Component: geometry Keywords: polyhedra, affine hull
Cc: jipilab, gh-LaisRast Merged in:
Authors: Jonathan Kliem Reviewers: Laith Rastanawi
Report Upstream: N/A Work issues:
Branch: 32e59c0 (Commits) Commit: 32e59c0ca60078ff7ed469ddc32686a1bb932542
Dependencies: Stopgaps:

Description (last modified by gh-kliem)

This ticket renames affine_hull of Polyhedron_base to affine_hull_projection and adds a deprecation warning.

Before, this function was hard to recognize, as by the name one expected to obtain the affine hull and not the polyhedron projected into it.

Also this ticket makes use of #28724 to simplify construction: We determine a transformation matrix A and a vector b and basically return A*P - b, where P is the polyhedron.

Change History (8)

comment:1 Changed 4 months ago by gh-kliem

  • Branch set to public/29326
  • Commit set to 6d12f79f482bf942d4602e7d8cb0bae896b0c7c4
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

f9e1283simplified projection into affine_hull
6d12f79rename `affine_hull` to `affine_hull_projection`

comment:2 Changed 4 months ago by gh-LaisRast

  • Reviewers set to Laith Rastanawi
  • Status changed from needs_review to needs_work
  • In polyhedra_quickref.rst:
    -    :meth:`~sage.geometry.polyhedron.base.Polyhedron_base.affine_hull_projection` | constructs an affinely equivalent full dimensional polyhedra
    +    :meth:`~sage.geometry.polyhedron.base.Polyhedron_base.affine_hull_projection` | constructs an affinely equivalent full-dimensional polyhedron
    
  • Make the second paragraph in the docstring look like a one paragraph.
  • In the "OUTPUT" section:
    -        A full-dimensional polyhedron or a linear transformation,
    +        A full-dimensional polyhedron or an affine transformation,
            depending on the parameter ``as_affine_map``.
    
  • Remove the following line from "TODO" section:
    -            - allow to return ``as_affine_map=True`` for default setting
    
  • The new name affine_hull_projection is definitively better than the old name, but I am not sure if this is the best name. How about full_dimensional_copy? I am also not sure about this suggestion. So maybe we need another opinion.

comment:3 follow-up: Changed 4 months ago by gh-kliem

The name has been discussed in https://groups.google.com/forum/#!topic/sage-devel/MEmAIPDPcHY If you want to propose a different name, please do so there, so that everyone is involved.

comment:4 in reply to: ↑ 3 Changed 4 months ago by gh-LaisRast

Replying to gh-kliem:

The name has been discussed in https://groups.google.com/forum/#!topic/sage-devel/MEmAIPDPcHY If you want to propose a different name, please do so there, so that everyone is involved.

I just wanted to hear some other opinions, and since many people already agreed for the name, I am also fine with it.

comment:5 Changed 4 months ago by git

  • Commit changed from 6d12f79f482bf942d4602e7d8cb0bae896b0c7c4 to 32e59c0ca60078ff7ed469ddc32686a1bb932542

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

32e59c0small changes

comment:6 Changed 4 months ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:7 Changed 4 months ago by gh-LaisRast

  • Status changed from needs_review to positive_review

It is good to go.

comment:8 Changed 3 months ago by vbraun

  • Branch changed from public/29326 to 32e59c0ca60078ff7ed469ddc32686a1bb932542
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.