#9954: f_vector outputs an extra top-dimensional cell
----------------------------------+-----------------------------------------
Reporter: schilly | Owner: mhampton
Type: defect | Status: needs_work
Priority: major | Milestone: sage-4.6.1
Component: geometry | Keywords:
Author: Volker Braun | Upstream: N/A
Reviewer: Andrey Novoseltsev | Merged:
Work_issues: |
----------------------------------+-----------------------------------------
Changes (by novoselt):
* status: needs_review => needs_work
Comment:
1. It seems that you wanted to cache the face lattice but forgot - the
output of `hasse_diagram_from_incidences` is directly returned.
1. Perhaps we should rename the method to `Hasse...`?
1. `PolyhedronFace` constructor does not describe input.
1. I think it would be nice to be able to treat polyhedron faces as
polyhedrons themselves, in the way how it is done for cones. This is
feasible only if creating new polyhedra is very fast, which is not the
case now, but maybe it is something to think about in the future. As an
easier to implement alternative, I would suggest adding a method
`polyhedron` to `PolyhedronFace` that will produce a corresponding
`Polyhedron` object. Note that this will clash with my desire to include
lines to all faces including the bottom one. I am not sure what is the
best option about it. Do you know what is the standard treatment in such
situations?
1. I think that new argument `faces_at_infinity` should be gone - it
seems obscure to me and it is unnecessary. Currently you have replaced my
line
{{{
head.extend(faces[frozenset([atom]), coatoms]
for atom, coatoms in enumerate(atom_to_coatoms))
}}}
with
{{{
for atom, coatoms in enumerate(atom_to_coatoms):
try:
head.append(faces[frozenset([atom]), coatoms])
except KeyError, msg:
if not faces_at_infinity:
raise KeyError, msg
}}}
but you can get the same result with
{{{
head.extend(faces[frozenset([atom]), coatoms]
for atom, coatoms in enumerate(atom_to_coatoms)
if atoms_require_one_of is None or atom in atoms_require_one_of)
}}}
1. How about renaming `atoms_require_one_of` to `required_atoms`? I agree
that your variant is more descriptive, but it sounds a bit weird as a
parameter name.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/9954#comment:6>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
