Opened 3 years ago

Closed 3 years ago

# Make outer normal fans readily available

Reported by: Owned by: nailuj major sage-8.9 geometry NormalFan, polytope, days100 jipilab Julian Ritter Jean-Philippe Labbé, Frédéric Chapoton N/A 440689c 440689c758f36b80c4dd0a9a88eee5c5d60f2b22

For a polytope `P`, both `NormalFan(P)` and `P.normal_fan()` return the inner normal fan having the inner facet normals as rays. The outer normal fan, using the outer facet normals, is well-known, but not as easy to find in Sage.

As an example, create the polytope on p.193 of Ziegler's "Lectures on Polytopes" with its normal fan:

```sage: V=[(-8,-4),(-8,6),(-4,-8),(0,-9),(0,6),(4,-8)]
sage: P=Polyhedron(V)
sage: NormalFan(P)
```

The result is the inner normal fan of `P`, which is the outer normal fan of `-P`.

This ticket adds to `Polyhedron.normal_fan` an argument `direction` set to either `'inner'` or `'outer'` to determine which of the directions to use. The default will stay `'inner'` in order to match the default of `NormalFan`. The description of `NormalFan` is extended by a hint to use `NormalFan(-P)` for the outer normal fan.

Additionally, this ticket adds in `sage.geometry.polyhedron.representation` a method `Inequality.outer_normal`. This allows to obtain the outer normal vector without having to think about the appropriate sign of `Inequality.A()` whenever dealing with the facet inequalities of a polyhedron.

### comment:1 Changed 3 years ago by nailuj

• Branch set to u/nailuj/normalfan_outer

### comment:2 Changed 3 years ago by nailuj

• Commit set to 484bf563c383e0b0fc85fd3e2af991dfb41a9932
• Status changed from new to needs_info

New commits:

 ​57d5508 `added direction argument to NormalFan` ​484bf56 `added outer_normal method`

### comment:3 Changed 3 years ago by embray

• Milestone changed from sage-8.8 to sage-8.9

Sage 8.8 is to be released fairly soon so unless this is a blocker this should be moved to the next release.

### comment:4 follow-up: ↓ 5 Changed 3 years ago by novoselt

I don't care much about adding `direction` argument, but I have extremely strong objection to making it the default. There was a reason why inner normals were used for many, many years (like since 2006) - it is consistent with the literature. I understand that some books/articles use the other way, but when neither way is exceptionally uncommon the default choice is pretty much random personal preference when you first create these classes and methods. Once they were created and used for a long time, there is absolutely no reason to change the default whatever it is.

### comment:5 in reply to: ↑ 4 Changed 3 years ago by jipilab

I don't care much about adding `direction` argument, but I have extremely strong objection to making it the default. There was a reason why inner normals were used for many, many years (like since 2006) - it is consistent with the literature. I understand that some books/articles use the other way, but when neither way is exceptionally uncommon the default choice is pretty much random personal preference when you first create these classes and methods. Once they were created and used for a long time, there is absolutely no reason to change the default whatever it is.

+1

I would leave the default as it is and add the `direction` argument to allow to get the variant.

### comment:6 follow-up: ↓ 7 Changed 3 years ago by novoselt

Actually, I think now it would be better to allow quick "direction flip" either on the polytope or the fan, i.e. make them support `-` operator, instead of making constructors more complicated.

### comment:7 in reply to: ↑ 6 Changed 3 years ago by jipilab

Actually, I think now it would be better to allow quick "direction flip" either on the polytope or the fan, i.e. make them support `-` operator, instead of making constructors more complicated.

This seems to already be possible for `Polyhedron` objects:

```sage: p = polytopes.regular_polygon(5)
sage: p.vertices()
(A vertex at (0.5877852522924731?, -0.8090169943749474?),
A vertex at (0.9510565162951536?, 0.3090169943749474?),
A vertex at (0, 1),
A vertex at (-0.5877852522924731?, -0.8090169943749474?),
A vertex at (-0.9510565162951536?, 0.3090169943749474?))
sage: (-p).vertices()
(A vertex at (-0.5877852522924731?, 0.8090169943749474?),
A vertex at (-0.9510565162951536?, -0.3090169943749474?),
A vertex at (0, -1),
A vertex at (0.587785252292474?, 0.8090169943749474?),
A vertex at (0.9510565162951536?, -0.3090169943749474?))
```

Or you have something else in mind?

Yes, I would use `NormalFan(-P)` or `NormalFan(P)` depending on whether one wants the inner or outer. This seems like the easiest thing to do. One does not need to go into the `Polyhedron` constructor...

Last edited 3 years ago by jipilab (previous) (diff)

### comment:8 follow-up: ↓ 9 Changed 3 years ago by novoselt

That's exactly what I meant. I think those who care about directions are likely to read the documentation and discover that these are inner normals. After that, if this is not the desired choice for the user, `NormalFan(-P)` seems easier and cleaner to use than `NormalFan(P, direction="outer")`.

### comment:9 in reply to: ↑ 8 Changed 3 years ago by jipilab

• Status changed from needs_info to needs_work

That's exactly what I meant. I think those who care about directions are likely to read the documentation and discover that these are inner normals. After that, if this is not the desired choice for the user, `NormalFan(-P)` seems easier and cleaner to use than `NormalFan(P, direction="outer")`.

I believe that the proposed change would be in the method `.normal_fan()` of `Polyhedron` objects, and not at the level of `NormalFan`, for the reason you mention.

That said, I would change the description of the ticket as follows:

I propose to add to `normal_fan` an argument direction set to either `'outer'` or `'inner'` to determine which of the directions to use. The default will stay inner in order to match the default of `NormalFan`.

Above I would keep the current default.

Additionally, I propose to add in sage.geometry.polyhedron.representation a method Inequality.outer_normal. This allows to obtain the outer normal vector without having to think about the appropriate sign of Inequality.A() whenever dealing with the facet inequalities of a polyhedron.

This is good to me.

Further, I would suggest to add

Use `NormalFan(-P)` to get the outer normal fan.

in the documentation of `NormalFan`.

### comment:10 Changed 3 years ago by git

• Commit changed from 484bf563c383e0b0fc85fd3e2af991dfb41a9932 to b41def5d69156640996e4a6e60d29d17b36bd435

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

 ​d8763cf `Revert "added direction argument to NormalFan"` ​7bfda6e `added hint to NormalFan(-P)` ​0c653c9 `added direction argument to Polyhedron.normal_fan` ​b41def5 `Merge branch 'develop' into normalfan_outer`

### comment:11 Changed 3 years ago by git

• Commit changed from b41def5d69156640996e4a6e60d29d17b36bd435 to fd3e89926e9648fb36bb2c087deb24b0c3938ef1

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

 ​fd3e899 `Fixed typo`

### comment:12 Changed 3 years ago by nailuj

• Description modified (diff)
• Status changed from needs_work to needs_review

That's exactly what I meant. I think those who care about directions are likely to read the documentation and discover that these are inner normals. After that, if this is not the desired choice for the user, `NormalFan(-P)` seems easier and cleaner to use than `NormalFan(P, direction="outer")`.

I believe that the proposed change would be in the method `.normal_fan()` of `Polyhedron` objects, and not at the level of `NormalFan`, for the reason you mention.

Yes. I stumbled upon this inner-outer issue through `Polyhedron.normal_fan()`. This is why I added jipilab in CC in the first place, and this is where I should have proposed to make `'outer'` the default – my bad. I'm fine with `'inner'` as default in both files. My main goal was easier access to outer normal fans, which is now available and explained in the docstrings.

That said, I would change the description of the ticket as follows:

I propose to add to `normal_fan` an argument direction set to either `'outer'` or `'inner'` to determine which of the directions to use. The default will stay inner in order to match the default of `NormalFan`.

Done.

Further, I would suggest to add

Use `NormalFan(-P)` to get the outer normal fan.

in the documentation of `NormalFan`.

Done.

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

• Reviewers set to Jean-Philippe Labbé
• Status changed from needs_review to needs_work
• The optional parameters should receive an appropriate doctest. For example:
```sage: p = polytopes.regular_polygon(4,base_ring=QQ).pyramid()
sage: inner_nf = p.normal_fan()
sage: inner_nf.rays()
N( 1,  0,  0),
N(-1,  1,  1),
N(-1,  1, -1),
N(-1, -1, -1),
N(-1, -1,  1)
in 3-d lattice N

sage: outer_nf = p.normal_fan(direction='outer')
sage: outer_nf.rays()
N(-1,  0,  0),
N( 1,  1, -1),
N( 1,  1,  1),
N( 1, -1,  1),
N( 1, -1, -1)
in 3-d lattice N
```
• Further, it would be nice to disallow such things:
```sage: outer_nf = p.normal_fan(direction='blabla')
```
by checking that direction is one of the two allowed things (see the volume function for a similar handling).
• Remove the space before the semi-colon in the docstring of `normal_fan`.

### comment:14 Changed 3 years ago by git

• Commit changed from fd3e89926e9648fb36bb2c087deb24b0c3938ef1 to 308dada78b84d1436dca28bd970e768d6ea6c42b

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

 ​257d367 `Added check that the direction is inner or outer` ​5ae5e68 `pyflakes fix` ​691046a `fixed typo` ​308dada `added examples`

### comment:15 in reply to: ↑ 13 Changed 3 years ago by nailuj

• Status changed from needs_work to needs_review

The optional parameters should receive an appropriate doctest. For example:

```sage: p = polytopes.regular_polygon(4,base_ring=QQ).pyramid()
sage: inner_nf = p.normal_fan()
sage: inner_nf.rays()
N( 1,  0,  0),
N(-1,  1,  1),
N(-1,  1, -1),
N(-1, -1, -1),
N(-1, -1,  1)
in 3-d lattice N

sage: outer_nf = p.normal_fan(direction='outer')
sage: outer_nf.rays()
N(-1,  0,  0),
N( 1,  1, -1),
N( 1,  1,  1),
N( 1, -1,  1),
N( 1, -1, -1)
in 3-d lattice N
```

I added a 2-d example testing inner, outer, and an invalid direction.

Further, it would be nice to disallow such things:

```sage: outer_nf = p.normal_fan(direction='blabla')
```

by checking that direction is one of the two allowed things (see the volume function for a similar handling).

Done.

Remove the space before the semi-colon in the docstring of `normal_fan`.

Done.

### comment:16 Changed 3 years ago by jipilab

Great!

Could you do one change: the backtick in the error message should be a single quote.

I would also fix the title of the ticket and the description to reflect the actual changes involved.

### comment:17 Changed 3 years ago by git

• Commit changed from 308dada78b84d1436dca28bd970e768d6ea6c42b to 9d20a1d2ef73c9fa4e89dc0657033191d22793fd

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

 ​9d20a1d `replaced backtick by single quote`

### comment:18 Changed 3 years ago by nailuj

• Description modified (diff)
• Summary changed from NormalFan: issue with inner and outer normal vectors to Make outer normal fans readily available

Could you do one change: the backtick in the error message should be a single quote.

Done.

I would also fix the title of the ticket and the description to reflect the actual changes involved.

Done.

### comment:19 Changed 3 years ago by jipilab

The doctest still has backticks.

### comment:20 Changed 3 years ago by git

• Commit changed from 9d20a1d2ef73c9fa4e89dc0657033191d22793fd to 440689c758f36b80c4dd0a9a88eee5c5d60f2b22

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

 ​440689c `replaced backticks in doctest`

### comment:21 Changed 3 years ago by chapoton

• Reviewers changed from Jean-Philippe Labbé to Jean-Philippe Labbé, Frédéric Chapoton
• Status changed from needs_review to positive_review

ok, c'est correct

### comment:22 Changed 3 years ago by vbraun

• Branch changed from u/nailuj/normalfan_outer to 440689c758f36b80c4dd0a9a88eee5c5d60f2b22
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.