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.

Sign up for GitHub

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

Jump to bottom

face_iter/face_generator of polyhedra: Specify algorithm instead of dual=False/True #33646

Closed
kliem opened this issue Apr 5, 2022 · 18 comments
Closed

face_iter/face_generator of polyhedra: Specify algorithm instead of dual=False/True #33646

kliem opened this issue Apr 5, 2022 · 18 comments
Labels
c: geometry p: major / 3 t: enhancement
Milestone
sage-9.6

Comments

@kliem
Copy link
Contributor

kliem commented Apr 5, 2022

In addition we add a function alias: face_generator for CombinatorialPolyhedron to make it consistent with Polyhedron_base.

We deprecate the keyword dual.

CC: @tscrim @yuan-zhou

Component: geometry

Author: Jonathan Kliem

Branch/Commit: ed15041

Reviewer: Travis Scrimshaw, Yuan Zhou

Issue created by migration from https://trac.sagemath.org/ticket/33646
@kliem kliem added this to the sage-9.6 milestone Apr 5, 2022
@kliem
Copy link
Contributor Author

kliem commented Apr 5, 2022

New commits:

04ca6fbfurther improvement for face iterator over simple/simplicial polytopes
3e8dd25allow algorithm for Polyhedron_base.face_generator
f638686prefer face_generator over face_iter and algorithm= over dual=

@kliem
Copy link
Contributor Author

kliem commented Apr 5, 2022

Commit: f638686

@kliem
Copy link
Contributor Author

kliem commented Apr 5, 2022

Branch: public/33646

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

01f30c5allow algorithm for Polyhedron_base.face_generator
88f4bcdprefer face_generator over face_iter and algorithm= over dual=

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2022

Changed commit from f638686 to 88f4bcd

@tscrim
Copy link
Collaborator

tscrim commented Apr 6, 2022

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Apr 6, 2022

comment:3

LGTM.

@yuan-zhou
Copy link

comment:4

I would suggest indicating in the docstring that algorithm option dominates dual if both are provided.

@yuan-zhou
Copy link

comment:6

And perhaps moving algorithm= before dual=, as the latter will be deprecated.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 6, 2022

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

ed15041deprecate keyword dual

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 6, 2022

Changed commit from 88f4bcd to ed15041

@kliem

This comment has been minimized.

Sign in to view
@kliem
Copy link
Contributor Author

kliem commented Apr 6, 2022

comment:8

I deprecated dual keyword altogether. If we are going to deprecate it, we might as well do it here.

@kliem kliem changed the title face_iter/face_generator of polyhedra: Allow to specify algorithm instead of dual=False/True face_iter/face_generator of polyhedra: Specify algorithm instead of dual=False/True Apr 6, 2022
@yuan-zhou
Copy link

comment:9

The changes look good to me. Thanks!
Waiting for the Patchbot.

@tscrim
Copy link
Collaborator

tscrim commented Apr 7, 2022

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Yuan Zhou

@tscrim
Copy link
Collaborator

tscrim commented Apr 7, 2022

comment:10

Patchbot failure is unrelated to this ticket.

@kliem
Copy link
Contributor Author

kliem commented Apr 7, 2022

comment:11

Thank you.

@vbraun
Copy link
Member

vbraun commented Apr 10, 2022

Changed branch from public/33646 to ed15041

@vbraun vbraun closed this as completed in 608a2dc Apr 10, 2022
@kliem kliem mentioned this issue May 20, 2022
Closed
mkoeppe added a commit to mkoeppe/sage that referenced this issue Apr 25, 2024
@mkoeppe
Polyhedron.face_generator: Remove keyword 'dual' deprecated in sagema…
c818e52 
…th#33646 (2022)
mkoeppe added a commit to mkoeppe/sage that referenced this issue Apr 25, 2024
@mkoeppe
CombinatorialPolyhedron.face_generator: Remove keyword 'dual' depreca…
8007822 
…ted in sagemath#33646 (2022)
mkoeppe added a commit to mkoeppe/sage that referenced this issue Apr 27, 2024
@mkoeppe
Polyhedron.face_generator: Remove keyword 'dual' deprecated in sagema…
8052b2c 
…th#33646 (2022)
mkoeppe added a commit to mkoeppe/sage that referenced this issue Apr 27, 2024
@mkoeppe
CombinatorialPolyhedron.face_generator: Remove keyword 'dual' depreca…
9826af1 
…ted in sagemath#33646 (2022)
mkoeppe added a commit to mkoeppe/sage that referenced this issue May 8, 2024
@mkoeppe
Polyhedron.face_generator: Remove keyword 'dual' deprecated in sagema…
2d5126c 
…th#33646 (2022)
mkoeppe added a commit to mkoeppe/sage that referenced this issue May 8, 2024
@mkoeppe
CombinatorialPolyhedron.face_generator: Remove keyword 'dual' depreca…
ab1630a 
…ted in sagemath#33646 (2022)
vbraun pushed a commit to vbraun/sage that referenced this issue May 9, 2024
sagemathgh-37869: sage.geometry.polyhedron: Remove deprecated code
8b963fc 
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Remove code deprecated in:
- sagemath#32592 (2021)
- sagemath#33646 (2022)
- sagemath#31834 (2021)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37869
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this issue May 11, 2024
sagemathgh-37869: sage.geometry.polyhedron: Remove deprecated code
5253a33 
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Remove code deprecated in:
- sagemath#32592 (2021)
- sagemath#33646 (2022)
- sagemath#31834 (2021)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37869
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this issue May 12, 2024
sagemathgh-37869: sage.geometry.polyhedron: Remove deprecated code
402c8da 
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Remove code deprecated in:
- sagemath#32592 (2021)
- sagemath#33646 (2022)
- sagemath#31834 (2021)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37869
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this issue May 12, 2024
sagemathgh-37869: sage.geometry.polyhedron: Remove deprecated code
58c4079 
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Remove code deprecated in:
- sagemath#32592 (2021)
- sagemath#33646 (2022)
- sagemath#31834 (2021)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37869
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: geometry p: major / 3 t: enhancement
Projects
None yet
Milestone
sage-9.6
Development

No branches or pull requests
4 participants
@vbraun @tscrim @yuan-zhou @kliem