Opened 2 years ago

Closed 2 years ago

Backend for Hyperplane Arrangements

Reported by: Owned by: jipilab major sage-9.2 geometry backend, normaliz, hyperplane, regions aram.dermenjian, gh-kliem, gh-LaisRast, selia, nailuj, tscrim Jean-Philippe Labbé Travis Scrimshaw N/A 927b707 927b707aa4cd44e76484a5bfc80057c879838e65

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'
```

comment:1 Changed 2 years ago by git

• Commit set to d2bdc288f054c95f272f78c997d7fefb9b44bece

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

 ​d2bdc28 `First version`

comment:2 Changed 2 years 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 2 years 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 2 years ago by jipilab

• Cc nailuj added

comment:5 follow-up: ↓ 7 Changed 2 years 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 2 years ago by tscrim

• Cc tscrim added

comment:7 in reply to: ↑ 5 Changed 2 years ago by jipilab

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 2 years ago by jipilab

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

New commits:

 ​7bb3b2c `First version` ​99ecd79 `Addons to documentation`

comment:9 Changed 2 years ago by git

• Commit changed from 99ecd79e337d97751d4423b381add5e0a089d22f to 49eed46afd9fb431cc666e99233ed0768d3b6e11

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

 ​9ace361 `Second version, more robust` ​7734ce9 `Made test pass` ​49eed46 `pep8`

comment:10 Changed 2 years ago by jipilab

• 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 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 2 years ago by git

• Commit changed from 49eed46afd9fb431cc666e99233ed0768d3b6e11 to 20816a8f4e3c4016cf616be52208e1efea9b5b7e

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

 ​20816a8 `Fix tests and comments`

comment:13 follow-up: ↓ 15 Changed 2 years ago by jipilab

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 2 years ago by jipilab

• Description modified (diff)

comment:15 in reply to: ↑ 13 ; follow-up: ↓ 17 Changed 2 years 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]]
```

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 git

• 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 ; follow-up: ↓ 18 Changed 2 years ago by jipilab

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 tscrim

• Reviewers set to Travis Scrimshaw
• Status changed from needs_review to positive_review

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 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.