Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements for projection into affine hull #29326

Closed
kliem opened this issue Mar 13, 2020 · 12 comments
Closed

Improvements for projection into affine hull #29326

kliem opened this issue Mar 13, 2020 · 12 comments

Comments

@kliem
Copy link
Contributor

kliem commented Mar 13, 2020

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.

CC: @jplab @LaisRast

Component: geometry

Keywords: polyhedra, affine hull

Author: Jonathan Kliem

Branch/Commit: 32e59c0

Reviewer: Laith Rastanawi

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

@kliem kliem added this to the sage-9.1 milestone Mar 13, 2020
@kliem
Copy link
Contributor Author

kliem commented Mar 13, 2020

Branch: public/29326

@kliem

This comment has been minimized.

@kliem
Copy link
Contributor Author

kliem commented Mar 13, 2020

New commits:

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

@kliem
Copy link
Contributor Author

kliem commented Mar 13, 2020

Commit: 6d12f79

@LaisRast
Copy link
Contributor

Reviewer: Laith Rastanawi

@LaisRast
Copy link
Contributor

comment:2
  • 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.

@kliem
Copy link
Contributor Author

kliem commented Mar 27, 2020

comment:3

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.

@LaisRast
Copy link
Contributor

comment:4

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2020

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

32e59c0small changes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2020

Changed commit from 6d12f79 to 32e59c0

@LaisRast
Copy link
Contributor

comment:7

It is good to go.

@vbraun
Copy link
Member

vbraun commented Apr 5, 2020

Changed branch from public/29326 to 32e59c0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants