Opened 12 months ago

Closed 9 months ago

#29506 closed enhancement (fixed)

Backend for Hyperplane Arrangements

Reported by: jipilab Owned by:
Priority: major Milestone: sage-9.2
Component: geometry Keywords: backend, normaliz, hyperplane, regions
Cc: aram.dermenjian, gh-kliem, gh-LaisRast, selia, nailuj, tscrim Merged in:
Authors: Jean-Philippe Labbé Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 927b707 (Commits, GitHub, GitLab) Commit: 927b707aa4cd44e76484a5bfc80057c879838e65
Dependencies: Stopgaps:

Status badges

Description (last modified by jipilab)

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^2-r9+1),0],[1,-r9^2-r9,0],
               [1,-r9^2+1,0],[1,-r9^2,0],[1,r9^2-4,-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 12 months ago by git

  • Commit set to d2bdc288f054c95f272f78c997d7fefb9b44bece

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

d2bdc28First version

comment:2 Changed 12 months ago by jipilab

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 12 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.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 12 months ago by jipilab

  • Cc nailuj added

comment:5 follow-up: Changed 12 months ago by 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. The other option I think could be good is implement a global option to hyperplane arrangements for a default polyhedron backend.

comment:6 Changed 12 months ago by tscrim

  • Cc tscrim added

comment:7 in reply to: ↑ 5 Changed 9 months ago by jipilab

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 gh-kliem 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 9 months ago by jipilab

  • Branch changed from u/jipilab/backend_ha to u/jipilab/backend_ha_rebase
  • Commit changed from d2bdc288f054c95f272f78c997d7fefb9b44bece to 99ecd79e337d97751d4423b381add5e0a089d22f

New commits:

7bb3b2cFirst version
99ecd79Addons to documentation

comment:9 Changed 9 months ago by git

  • Commit changed from 99ecd79e337d97751d4423b381add5e0a089d22f to 49eed46afd9fb431cc666e99233ed0768d3b6e11

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

9ace361Second version, more robust
7734ce9Made test pass
49eed46pep8

comment:10 Changed 9 months ago by jipilab

  • Status changed from new to needs_review

Here's a second version. Let me know what you think.

comment:11 Changed 9 months ago by tscrim

Thank you. A few little things:

-            sage: norms = [[1,1/3*(-2*r9**2-r9+1),0], \
-                           [1,-r9**2-r9,0], \
-                           [1,-r9**2+1,0], \
-                           [1,-r9**2,0], \
-                           [1,r9**2-4,-r9**2+3]]
+            sage: norms = [[1,1/3*(-2*r9**2-r9+1),0],
+            ....:          [1,-r9**2-r9,0],
+            ....:          [1,-r9**2+1,0],
+            ....:          [1,-r9**2,0],
+            ....:          [1,r9**2-4,-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 9 months ago by git

  • Commit changed from 49eed46afd9fb431cc666e99233ed0768d3b6e11 to 20816a8f4e3c4016cf616be52208e1efea9b5b7e

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

20816a8Fix tests and comments

comment:13 follow-up: Changed 9 months ago by jipilab

Replying to tscrim:

Thank you. A few little things:

-            sage: norms = [[1,1/3*(-2*r9**2-r9+1),0], \
-                           [1,-r9**2-r9,0], \
-                           [1,-r9**2+1,0], \
-                           [1,-r9**2,0], \
-                           [1,r9**2-4,-r9**2+3]]
+            sage: norms = [[1,1/3*(-2*r9**2-r9+1),0],
+            ....:          [1,-r9**2-r9,0],
+            ....:          [1,-r9**2+1,0],
+            ....:          [1,-r9**2,0],
+            ....:          [1,r9**2-4,-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 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.

Done.

comment:14 Changed 9 months ago by jipilab

  • Description modified (diff)

comment:15 in reply to: ↑ 13 ; follow-up: Changed 9 months ago by tscrim

Replying to jipilab:

Replying to tscrim:

Thank you. A few little things:

-            sage: norms = [[1,1/3*(-2*r9**2-r9+1),0], \
-                           [1,-r9**2-r9,0], \
-                           [1,-r9**2+1,0], \
-                           [1,-r9**2,0], \
-                           [1,r9**2-4,-r9**2+3]]
+            sage: norms = [[1,1/3*(-2*r9**2-r9+1),0],
+            ....:          [1,-r9**2-r9,0],
+            ....:          [1,-r9**2+1,0],
+            ....:          [1,-r9**2,0],
+            ....:          [1,r9**2-4,-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 9 months ago by git

  • Commit changed from 20816a8f4e3c4016cf616be52208e1efea9b5b7e to 927b707aa4cd44e76484a5bfc80057c879838e65

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

927b707added dots

comment:17 in reply to: ↑ 15 ; follow-up: Changed 9 months ago by jipilab

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 9 months ago by tscrim

  • 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 9 months ago by vbraun

  • Branch changed from u/jipilab/backend_ha_rebase to 927b707aa4cd44e76484a5bfc80057c879838e65
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.