Add PolyhedralComplex as new object #887
Conversation
|
Looking at https://github.com/oscar-system/Oscar.jl/actions/runs/1558072926 it talks about merging? Was it trying to merge my draft? |
|
Yes, it always runs the tests on a merge into the master branch. The failure was just a network hiccup. |
lkastner
force-pushed
the
lk/polyhedralComplex
branch
from
0c2c5b9
to
4fd49cf
Compare
| ############################################################################### | ||
| function Base.show(io::IO, PC::PolyhedralComplex) | ||
| ad = ambient_dim(PC) | ||
| if ad == -1.0 |
There was a problem hiding this comment.
I don't understand this line of code: (1) it should not be possible to create a polyhedral complex without an ambient dimension. (2) Why is the right hand side -1.0 (and not an integer)?
There was a problem hiding this comment.
Could we not make a polyhedral complex just from a Hasse diagram? @YueRen says he needs polyhedral complexes that aren't embedded. We also had polyhedra without embedding. This right hand side stems from the fact that polymake would not error, but instead return -1.0 if the ambient dimension could not be computed. The code is the same for the polyhedron, maybe this is fixed now, @benlorenz ?
There was a problem hiding this comment.
I fixed the code in polymake for Cone, Polytope and PolyhedralFan but somehow missed PolyhedralComplex. I will add the missing input property for the upcoming release, after that change the ambient dim method should fail instead of returning a plain perl number. The code tries to compute FAN_AMBIENT_DIM - 1 which should be an int but in this case it is undef - 1 which perl evaluates to a float (perl number type) -1. (This is then translated to a julia Float)
src/PolyhedralGeometry/PolyhedralComplex/standard_constructions.jl
Outdated
Show resolved
Hide resolved
| @@ -42,6 +42,7 @@ ambient_dim(PC::PolyhedralComplex) | |||
| codim(PC::PolyhedralComplex) | |||
| dim(PC::PolyhedralComplex) | |||
| f_vector(PC::PolyhedralComplex) | |||
| isembedded(PC::PolyhedralComplex) | |||
There was a problem hiding this comment.
Is there a specific reason for this function?
According to your definition in the doc a polyhedral complex is always embedded.
There was a problem hiding this comment.
@YueRen needs complexes that aren't embedded, where we e.g. only know the Hasse diagram. I was going to say that we also have polyhedra that aren't embedded, but they seem to be gone? The definition seems now always to be embedded.
| $\mathbb{F}^n$, for $n$ fixed, is a *polyhedral complex* if | ||
|
|
||
| - the set $\mathcal{F}$ is closed with respect to taking faces and | ||
| - if $C,D\in\mathcal{F}$ then $C\cap D$ is a face of both, $C$ and $D$. |
There was a problem hiding this comment.
| - if $C,D\in\mathcal{F}$ then $C\cap D$ is a face of both, $C$ and $D$. | |
| - if $C,D\in\mathcal{F}$ then $C\cap D$ is a face of both $C$ and $D$. |
Seems this PR was extracted from PR #889 so I guess some more of the comments I made there apply here (and should probably be addressed here).
Alas, this PR here also is marked as a draft. So I am not sure whether you actually want feedback right now, or not, or what?!
There was a problem hiding this comment.
I am unsure why I am being questioned in this way. I marked both this and #889 as drafts since they are not finished. You are of course free to give feedback, but you might remark on things that we intend to change anyway. I thought marking a PR as draft implies that feedback at this point is not needed yet.
There was a problem hiding this comment.
Hi Lars, sorry, only saw your comment here just now. To be clear: I did not mean to "question" you (like an inquisitor), to the contrary: what I meant was this: I saw the one PR and had some immediate feedback. Then I saw this PR. Then I noticed that they are marked as draft, and now was like "oh, ok, maybe he doesn't even want feedback?!?".
My apologies for very badly communicating this!
lkastner
force-pushed
the
lk/polyhedralComplex
branch
from
df049ed
to
4fea7cd
Compare
…e object eliminated
…m a polyhedral complex
- Fix some renaming which was forgotten in exports - Add new functions to docs - nrays, nvertices, npolyhedra, f_vector added
…plex(maximal_polyhedra(PC)) work properly
lkastner
force-pushed
the
lk/polyhedralComplex
branch
from
4fea7cd
to
44b637f
Compare
lkastner
force-pushed
the
lk/polyhedralComplex
branch
from
2d59192
to
1033249
Compare
Previously Polymake.jl did not give an error when the ambient dimension could not be computed, instead it returned -1.0. This has now changed, so we can deal with this properly in the show method.
lkastner
force-pushed
the
lk/polyhedralComplex
branch
from
1033249
to
71c8a6d
Compare
lkastner
force-pushed
the
lk/polyhedralComplex
branch
from
dee859e
to
afbf760
Compare
Since we have cones and fans (and polyhedra), it makes sense to complete this.
Polyhedral complexes are needed in the tropical geometry part to come, nevertheless it seems sensible to have a separate PR.