Opened 2 years ago
Closed 2 years ago
#29506 closed enhancement (fixed)
Backend for Hyperplane Arrangements
Reported by:  jipilab  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  geometry  Keywords:  backend, normaliz, hyperplane, regions 
Cc:  aram.dermenjian, ghkliem, ghLaisRast, selia, nailuj, tscrim  Merged in:  
Authors:  JeanPhilippe Labbé  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  927b707 (Commits, GitHub, GitLab)  Commit:  927b707aa4cd44e76484a5bfc80057c879838e65 
Dependencies:  Stopgaps: 
Description (last modified by )
Currently, Hyperplane arrangements only use the default backends for related polyhedral objects.
We should make it possible to use backends. For example:
sage: K.<q> = CyclotomicField(9) sage: L.<r9> = NumberField((q+q^(1)).minpoly(),embedding = AA(q+q^1)) sage: norms = [[1,1/3*(2*r9^2r9+1),0],[1,r9^2r9,0], [1,r9^2+1,0],[1,r9^2,0],[1,r9^24,r9^2+3]] sage: H.<x,y,z> = HyperplaneArrangements(L) sage: A = H(backend='normaliz') sage: for v in norms: ....: a,b,c = v ....: A = A.add_hyperplane(a*x + b*y + c*z) sage: R = A.regions() # optional  pynormaliz sage: R[0].backend() # optional  pynormaliz 'normaliz'
Change History (19)
comment:1 Changed 2 years ago by
 Commit set to d2bdc288f054c95f272f78c997d7fefb9b44bece
comment:2 Changed 2 years ago by
This is just a first version to make it work with regions.
Probably, one should think about whether it is desirable to have an underlying backend when one deals with an hyperplane arrangement... Because it is annoying to add it as an option in each method...
comment:3 Changed 2 years ago by
 Milestone changed from sage9.1 to sage9.2
Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.
comment:4 Changed 2 years ago by
 Cc nailuj added
comment:5 followup: ↓ 7 Changed 2 years ago by
Not everything you would do with a hyperplane arrangement requires the polyhedron code. So I am a little more inclined to keep it being an option to each method than something passed to the consturction. The other option I think could be good is implement a global option to hyperplane arrangements for a default polyhedron backend.
comment:6 Changed 2 years ago by
 Cc tscrim added
comment:7 in reply to: ↑ 5 Changed 2 years ago by
Replying to tscrim:
Not everything you would do with a hyperplane arrangement requires the polyhedron code. So I am a little more inclined to keep it being an option to each method than something passed to the consturction.
I agree. One issue discussed with ghkliem is the case that one has two methods that use stored polyhedra, but then the desired backend is not the one of the precomputed data. I don't want to start dealing with this inside of hyperplane arrangement, exactly for the reason you mentioned: this should be dealt with by polyhedra somehow...
The other option I think could be good is implement a global option to hyperplane arrangements for a default polyhedron backend.
Yes, this sounds reasonable, simply have an option that specifies the wished backend to do the polyhedral computation. This makes sense to me. I'm starting to make these changes.
comment:8 Changed 2 years ago by
 Branch changed from u/jipilab/backend_ha to u/jipilab/backend_ha_rebase
 Commit changed from d2bdc288f054c95f272f78c997d7fefb9b44bece to 99ecd79e337d97751d4423b381add5e0a089d22f
comment:9 Changed 2 years ago by
 Commit changed from 99ecd79e337d97751d4423b381add5e0a089d22f to 49eed46afd9fb431cc666e99233ed0768d3b6e11
comment:10 Changed 2 years ago by
 Status changed from new to needs_review
Here's a second version. Let me know what you think.
comment:11 Changed 2 years ago by
Thank you. A few little things:
 sage: norms = [[1,1/3*(2*r9**2r9+1),0], \  [1,r9**2r9,0], \  [1,r9**2+1,0], \  [1,r9**2,0], \  [1,r9**24,r9**2+3]] + sage: norms = [[1,1/3*(2*r9**2r9+1),0], + ....: [1,r9**2r9,0], + ....: [1,r9**2+1,0], + ....: [1,r9**2,0], + ....: [1,r9**24,r9**2+3]]
 ``backend``  string (optional; default: ``None``); the backend to  use for the related polyhedral objects. + use for the related polyhedral objects
OUTPUT:  A string giving the backend or None (default) + A string giving the backend or ``None`` if none is specified.
Less important, but because I saw it when you changed the line: There is also a global instance of Fields()
nailed in memory under the name _Fields
(see rings/ring.pyx
; although this seems to be done in multiple places...), so I would import that and do
 if base_ring not in Fields: + if base_ring not in _Fields:
because it is a faster check.
comment:12 Changed 2 years ago by
 Commit changed from 49eed46afd9fb431cc666e99233ed0768d3b6e11 to 20816a8f4e3c4016cf616be52208e1efea9b5b7e
Branch pushed to git repo; I updated commit sha1. New commits:
20816a8  Fix tests and comments

comment:13 followup: ↓ 15 Changed 2 years ago by
Replying to tscrim:
Thank you. A few little things:
 sage: norms = [[1,1/3*(2*r9**2r9+1),0], \  [1,r9**2r9,0], \  [1,r9**2+1,0], \  [1,r9**2,0], \  [1,r9**24,r9**2+3]] + sage: norms = [[1,1/3*(2*r9**2r9+1),0], + ....: [1,r9**2r9,0], + ....: [1,r9**2+1,0], + ....: [1,r9**2,0], + ....: [1,r9**24,r9**2+3]]
Removing the backslashes, I get a SyntaxError?, EOF reached... I get get my head around it. I removed them, let's see if the bots react differently.
 ``backend``  string (optional; default: ``None``); the backend to  use for the related polyhedral objects. + use for the related polyhedral objectsOUTPUT:  A string giving the backend or None (default) + A string giving the backend or ``None`` if none is specified.Less important, but because I saw it when you changed the line: There is also a global instance of
Fields()
nailed in memory under the name_Fields
(seerings/ring.pyx
; although this seems to be done in multiple places...), so I would import that and do if base_ring not in Fields: + if base_ring not in _Fields:because it is a faster check.
Done.
comment:14 Changed 2 years ago by
 Description modified (diff)
comment:15 in reply to: ↑ 13 ; followup: ↓ 17 Changed 2 years ago by
Replying to jipilab:
Replying to tscrim:
Thank you. A few little things:
 sage: norms = [[1,1/3*(2*r9**2r9+1),0], \  [1,r9**2r9,0], \  [1,r9**2+1,0], \  [1,r9**2,0], \  [1,r9**24,r9**2+3]] + sage: norms = [[1,1/3*(2*r9**2r9+1),0], + ....: [1,r9**2r9,0], + ....: [1,r9**2+1,0], + ....: [1,r9**2,0], + ....: [1,r9**24,r9**2+3]]Removing the backslashes, I get a SyntaxError?, EOF reached... I get get my head around it. I removed them, let's see if the bots react differently.
Note the added ....:
.
BTW  How have you been doing?
comment:16 Changed 2 years ago by
 Commit changed from 20816a8f4e3c4016cf616be52208e1efea9b5b7e to 927b707aa4cd44e76484a5bfc80057c879838e65
Branch pushed to git repo; I updated commit sha1. New commits:
927b707  added dots

comment:17 in reply to: ↑ 15 ; followup: ↓ 18 Changed 2 years ago by
Replying to tscrim:
Note the added
....:
.
(facepalm)!
BTW  How have you been doing?
I've been finishing up my Habilitation in the spring (finished a month ago!), then took some vacation. Now slowly back to business! Things are good! How are things for you?
comment:18 in reply to: ↑ 17 Changed 2 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Replying to jipilab:
Replying to tscrim:
Note the added
....:
.(facepalm)!
Thanks. LGTM.
BTW  How have you been doing?
I've been finishing up my Habilitation in the spring (finished a month ago!), then took some vacation. Now slowly back to business! Things are good! How are things for you?
Congratulations! Hopefully COVID didn't prohibit you from having a good vacation. I am doing well; still at UQ. I just finished teaching last Friday, and now to do a bit of a research and Sage push.
comment:19 Changed 2 years ago by
 Branch changed from u/jipilab/backend_ha_rebase to 927b707aa4cd44e76484a5bfc80057c879838e65
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
First version