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

sage.geometry.polyhedron: Split out backend_cdd_rdf from backend_cdd #32592

Closed
mkoeppe opened this issue Sep 30, 2021 · 25 comments
Closed

sage.geometry.polyhedron: Split out backend_cdd_rdf from backend_cdd #32592

mkoeppe opened this issue Sep 30, 2021 · 25 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 30, 2021

(cherry-picked from #32432)

CC: @kliem

Component: geometry

Author: Matthias Koeppe

Branch/Commit: 57919d4

Reviewer: Jonathan Kliem

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

@mkoeppe mkoeppe added this to the sage-9.5 milestone Sep 30, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 30, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 30, 2021

Commit: e251555

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 30, 2021

New commits:

e251555src/sage/geometry/polyhedron/backend_cdd_rdf.py: Split out from src/sage/geometry/polyhedron/backend_cdd.py

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 30, 2021

Author: Matthias Koeppe

@kliem
Copy link
Contributor

kliem commented Sep 30, 2021

comment:3

Do I get this right that RDF will not be a "dependency" of sagemath-polyhedra?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 30, 2021

comment:4

The final design of the sagemath-polyhedra is not set in stone. But yes, for now I excluded RDF because of the compile time dependency on gsl and cypari2.

@kliem
Copy link
Contributor

kliem commented Sep 30, 2021

comment:5

I find it confusing that the import is at the end of the file backend_cdd.py.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 30, 2021

comment:6

I've now changed it to a standard "lazy import with deprecation"

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2021

Changed commit from e251555 to 7f317f1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2021

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

5ec8fa8src/sage/geometry/polyhedron/backend_cdd.py: Make Polyhedron_RDF_cdd a deprecated lazy_import
7f317f1src/sage/geometry/polyhedron/parent.py: Use lazy_import for Polyhedron_RDF_cdd

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2021

Changed commit from 7f317f1 to 4ec3829

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2021

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

4ec3829src/sage/geometry/polyhedron/backend_cdd_rdf.py: Update imports in doctests

@kliem
Copy link
Contributor

kliem commented Oct 1, 2021

comment:9

Docbuild fails

[dochtml] Build finished. The built documents can be found in /home/sagemath/sage-9.1/local/share/doc/sage/inventory/en/reference/tensor_free_modules
[dochtml] cd /home/sagemath/sage-9.1 && ./sage --docbuild --no-pdf-links reference/combinat inventory --no-prune-empty-dirs
[dochtml] Error building the documentation.
[dochtml] Traceback (most recent call last):
[dochtml]   File "/usr/lib/python3.7/runpy.py", line 193, in _run_module_as_main
[dochtml]     "__main__", mod_spec)
[dochtml]   File "/usr/lib/python3.7/runpy.py", line 85, in _run_code
[dochtml]     exec(code, run_globals)
[dochtml]   File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage_docbuild/__main__.py", line 2, in <module>
[dochtml]     main()
[dochtml]   File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage_docbuild/__init__.py", line 1814, in main
[dochtml]     builder()
[dochtml]   File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage_docbuild/__init__.py", line 810, in _wrapper
[dochtml]     self.write_auto_rest_file(module_name)
[dochtml]   File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage_docbuild/__init__.py", line 1105, in write_auto_rest_file
[dochtml]     title = self.get_module_docstring_title(module_name)
[dochtml]   File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage_docbuild/__init__.py", line 1066, in get_module_docstring_title
[dochtml]     __import__(module_name)
[dochtml]   File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/combinat/root_system/associahedron.py", line 27, in <module>
[dochtml]     from sage.geometry.polyhedron.parent import Polyhedra, Polyhedra_base, Polyhedra_QQ_ppl, Polyhedra_QQ_normaliz, Polyhedra_QQ_cdd, Polyhedra_polymake, Polyhedra_field
[dochtml]   File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/geometry/polyhedron/parent.py", line 1131, in <module>
[dochtml]     lazy_import('sage.geometry.polyhedron.backend_cdd_rdf', Polyhedron_RDF_cdd)
[dochtml] NameError: name 'lazy_import' is not defined
[dochtml] 
[dochtml]     Note: incremental documentation builds sometimes cause spurious
[dochtml]     error messages. To be certain that these are real errors, run
[dochtml]     "make doc-clean" first and try again.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 1, 2021

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

1a845fasrc/sage/geometry/polyhedron/parent.py: Add missing import

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 1, 2021

Changed commit from 4ec3829 to 1a845fa

@kliem
Copy link
Contributor

kliem commented Oct 1, 2021

comment:11

Still fails:

[dochtml]   File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/geometry/polyhedron/parent.py", line 1132, in <module>
[dochtml]     lazy_import('sage.geometry.polyhedron.backend_cdd_rdf', Polyhedron_RDF_cdd)
[dochtml] NameError: name 'Polyhedron_RDF_cdd' is not defined

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 1, 2021

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

27a265csrc/sage/geometry/polyhedron/parent.py: Fixup

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 1, 2021

Changed commit from 1a845fa to 27a265c

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 1, 2021

comment:13

Now there's a green checkmark

@kliem
Copy link
Contributor

kliem commented Oct 2, 2021

Reviewer: Jonathan Kliem

@kliem
Copy link
Contributor

kliem commented Oct 2, 2021

comment:14

LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 2, 2021

comment:15

Thank you!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 11, 2021

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

57919d4Merge tag '9.5.beta3' into t/32592/sage_geometry_polyhedron__split_out_backend_cdd_rdf_from_backend_cdd

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 11, 2021

Changed commit from 27a265c to 57919d4

@vbraun
Copy link
Member

vbraun commented Oct 13, 2021

@vbraun vbraun closed this as completed in 67da874 Oct 13, 2021
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 12, 2023
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 12, 2023
mkoeppe added a commit to mkoeppe/sage that referenced this issue Apr 25, 2024
mkoeppe added a commit to mkoeppe/sage that referenced this issue Apr 27, 2024
mkoeppe added a commit to mkoeppe/sage that referenced this issue May 8, 2024
vbraun pushed a commit to vbraun/sage that referenced this issue May 9, 2024
    
<!-- ^ 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
    
<!-- ^ 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
    
<!-- ^ 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
    
<!-- ^ 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
Projects
None yet
Development

No branches or pull requests

3 participants