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

Split initialization and basic access of polyhedra out as a separate module #32767

Closed
kliem opened this issue Oct 25, 2021 · 17 comments
Closed

Comments

@kliem
Copy link
Contributor

kliem commented Oct 25, 2021

Part of #32651.

Outsource initialization and very basic access (Vrepresentation, Hrepresentation, backend, base_ring) to a base class Polyhedron_base0.

Depends on #32637

CC: @kliem @jplab @tscrim @mkoeppe

Component: geometry

Author: Jonathan Kliem

Branch/Commit: 078cc56

Reviewer: Matthias Koeppe

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

@kliem kliem added this to the sage-9.5 milestone Oct 25, 2021
@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 25, 2021

comment:2

Should dim, ambient_dim rather be in base_convex?

@kliem
Copy link
Contributor Author

kliem commented Oct 25, 2021

comment:3

I think your suggestion makes more sense. They are not needed there yet, except for __repr__, which should also move up, as it isn't complete then.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 25, 2021

Changed commit from 9a48bd9 to 6dfadb8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 25, 2021

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

6dfadb8leave dim and ambient_dim for convex set

@kliem
Copy link
Contributor Author

kliem commented Oct 25, 2021

comment:5

Regarding the names, I don't know what makes most sense. I was thinking base0.py etc., as it is clear then, what the order is. However, we are not doing cython, so we don't need to be strict with inheritance and could use names also and try to have it more or less consistent.

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 25, 2021

comment:6

The random failure

sage -t --long --random-seed=77478494819088915365500074763386376542 src/sage/rings/continued_fraction.py
**********************************************************************
File "src/sage/rings/continued_fraction.py", line 265, in sage.rings.continued_fraction.rat_interval_cf_list

is probably not from this ticket.

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 25, 2021

comment:7

base0 is fine, I think

@mkoeppe mkoeppe changed the title Split initialization and basic access of polyhedra into seperate module Split initialization and basic access of polyhedra out as a separate module Oct 25, 2021
@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 25, 2021

comment:9

pyflakes reports:

src/sage/geometry/polyhedron/base.py:42:1 'sage.misc.superseded.deprecated_function_alias' imported but unused

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 25, 2021

Dependencies: #32637

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 25, 2021

Changed branch from u/gh-kliem/polyhedron_base0 to u/mkoeppe/polyhedron_base0

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 25, 2021

comment:12

Merged #32637 to avoid merge conflict.


Last 10 new commits:

eeaa722Merge #32614
c872d69Add description to features
08248a1Fix for doctest failures
759c88bsage.doctest, sage.control: Remove unused imports
15729dcsrc/sage/features/mcqd.py: Add doctests
1d02bd0More doctests for coverage
58572e1Merge #32649
44540f5src/sage/features/sagemath.py: Add doctests
a34658eMerge #32637
301a5e0src/sage/geometry/polyhedron/base.py: Remove unused import

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 25, 2021

Changed commit from 6dfadb8 to 301a5e0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 25, 2021

Changed commit from 301a5e0 to 078cc56

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 25, 2021

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

b8c3ac7Merge #32637
078cc56src/sage/geometry/polyhedron/base.py: Remove unused import

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 25, 2021

Reviewer: Matthias Koeppe

@kliem
Copy link
Contributor Author

kliem commented Oct 26, 2021

comment:15

Thank you.

@vbraun
Copy link
Member

vbraun commented Oct 31, 2021

Changed branch from u/mkoeppe/polyhedron_base0 to 078cc56

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