Opened 9 months ago
Closed 9 months ago
#30204 closed enhancement (fixed)
Prepare Polyhedra parent factory to handle more general ambient spaces
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  geometry  Keywords:  
Cc:  ghkliem, jipilab  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Jonathan Kliem 
Report Upstream:  N/A  Work issues:  
Branch:  ab7c5ff (Commits, GitHub, GitLab)  Commit:  ab7c5ff933880df5fd645fcbc82acb25b667fe88 
Dependencies:  Stopgaps: 
Description (last modified by )
This adds a new way to set up a Polyhedra parent.
sage: V = VectorSpace(QQ, 3) sage: Polyhedra(V) is Polyhedra(QQ, 3) True
Part of #30198.
Change History (9)
comment:1 Changed 9 months ago by
 Branch set to u/mkoeppe/generalize_polyhedra_parent_factory_to_handle_more_general_ambient_spaces
comment:2 Changed 9 months ago by
 Commit set to 20082d5132efccf048686b596b24700465dd2d57
 Description modified (diff)
 Status changed from new to needs_review
 Summary changed from Generalize Polyhedra parent factory to handle more general ambient spaces to Prepare Polyhedra parent factory to handle more general ambient spaces
comment:3 Changed 9 months ago by
sage: V = FiniteRankFreeModule(QQ, 2) sage: Polyhedra(V)  KeyError Traceback (most recent call last) /home/jonathan/Applications/sage/local/lib/python3.7/sitepackages/sage/structure/category_object.pyx in sage.structure.category_object.CategoryObject.getattr_from_category (build/cythonized/sage/structure/category_object.c:6954)() 837 try: > 838 return self.__cached_methods[name] 839 except KeyError: KeyError: 'dimension' During handling of the above exception, another exception occurred: AttributeError Traceback (most recent call last) <ipythoninput300c140d68c4dd> in <module>() > 1 Polyhedra(V) /home/jonathan/Applications/sage/local/lib/python3.7/sitepackages/sage/geometry/polyhedron/parent.py in Polyhedra(ambient_space_or_base_ring, ambient_dim, backend, ambient_space, base_ring) 113 base_ring = ambient_space.base_ring() 114 if ambient_dim is None: > 115 ambient_dim = ambient_space.dimension() 116 if ambient_space is not FreeModule(base_ring, ambient_dim): 117 raise NotImplementedError('ambient_space must be standard free module') /home/jonathan/Applications/sage/local/lib/python3.7/sitepackages/sage/structure/category_object.pyx in sage.structure.category_object.CategoryObject.__getattr__ (build/cythonized/sage/structure/category_object.c:6876)() 830 AttributeError: 'PrimeNumbers_with_category' object has no attribute 'sadfasdf' 831 """ > 832 return self.getattr_from_category(name) 833 834 cdef getattr_from_category(self, name): /home/jonathan/Applications/sage/local/lib/python3.7/sitepackages/sage/structure/category_object.pyx in sage.structure.category_object.CategoryObject.getattr_from_category (build/cythonized/sage/structure/category_object.c:7039)() 845 cls = self._category.parent_class 846 > 847 attr = getattr_from_other_class(self, cls, name) 848 self.__cached_methods[name] = attr 849 return attr /home/jonathan/Applications/sage/local/lib/python3.7/sitepackages/sage/cpython/getattr.pyx in sage.cpython.getattr.getattr_from_other_class (build/cythonized/sage/cpython/getattr.c:2548)() 387 dummy_error_message.cls = type(self) 388 dummy_error_message.name = name > 389 raise AttributeError(dummy_error_message) 390 cdef PyObject* attr = instance_getattr(cls, name) 391 if attr is NULL: AttributeError: 'FiniteRankFreeModule_with_category' object has no attribute 'dimension'
I think we should add dimension
as an alias for rank
in FiniteRankFreeModule
.
comment:4 followup: ↓ 5 Changed 9 months ago by
Thanks for catching this. I'm working on an improvement and will add some more tests as well.
Let's not do changes to the modules API on this ticket.
comment:5 in reply to: ↑ 4 Changed 9 months ago by
comment:6 Changed 9 months ago by
 Commit changed from 20082d5132efccf048686b596b24700465dd2d57 to ab7c5ff933880df5fd645fcbc82acb25b667fe88
Branch pushed to git repo; I updated commit sha1. New commits:
ab7c5ff  Polyhedra: Handle ambient_space = a free module correctly

comment:7 Changed 9 months ago by
 Reviewers set to Jonathan Kliem
For CombinatorialFreeModule
and FiniteRankFreeModule
I'm getting both NotImplementedError: ambient_space must be a standard free module
.
I assume that #30094 takes care of this?
If this is the case, you can put this on positive review on my behalf.
comment:8 Changed 9 months ago by
 Status changed from needs_review to positive_review
comment:9 Changed 9 months ago by
 Branch changed from u/mkoeppe/generalize_polyhedra_parent_factory_to_handle_more_general_ambient_spaces to ab7c5ff933880df5fd645fcbc82acb25b667fe88
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
sage.geometry.polyhedron.parent.Polyhedra: Generalize the factory