#18213: A lot of polytopes constructor are broken
-------------------------------------+-------------------------------------
Reporter: vdelecroix | Owner:
Type: defect | Status: needs_review
Priority: major | Milestone: sage-6.7
Component: geometry | Resolution:
Keywords: | Merged in:
Authors: Vincent Delecroix | Reviewers:
Report Upstream: N/A | Work issues:
Branch: | Commit:
u/vdelecroix/18213 | 524f00ea3d1d3734a9915cd6052d0bf677fdb6e3
Dependencies: #18211 | Stopgaps:
-------------------------------------+-------------------------------------
Comment (by ncohen):
Here are some comments. Please understand that doing everything at once
makes things *MUCH* harder to understand for the reviewer.
Improving the implementation and improving the doc could have been two
separate commits. The work that you do on each function could have been a
separate commit. Removing deprecations could have been a separate commit.
There are also things that anybody can review (only code) and things for
which you need to be used to the tools you deal with.
- The projection is not the orthogona projection I expected:
{{{
sage: sage: zero_sum_projection(3)*vector([1,0,0])
(0.7071067811865475, 0.4082482904638631)
}}}
You can either change the matrix or emphasize in the docstring that the
projection is not unique. Something like "returns a (d-1)xd matrix of
rank d
whose kernel contain (1,...,1). At first sight, I expected the function
to
return a matrix of dimensions `d\times d`. I updated the docstring on
this
matter.
- I do not understand the paragraph in the doc of `project_points` but
that's
probably because I don't know the maths behind.
- {{{This projection is the only one compatible with the restriction to
the
first coordinates}}} -- compatible?
- {{{Its volume is `\sqrt{d+1} / d!`}}} -- you should probably add a `#
tol`
around there. Or even better, check that the difference between the two
is 0
(still, with some tolerance)
- At this point I still do not know if it is very wise to use this
definition of
the projection, but it would be cool if every function which uses this
projection (like !Simplex) could redirect toward the matrix function.
This way
people would have a chance to learn what exactly they get as a result.
- {{{
sage: polytopes.icosahedron(base_ring=QQ)
...
TypeError: unable to convert 1/4*sqrt(5) + 1/4 to a rational
}}}
I added a small commit at public/18213
Nathann
--
Ticket URL: <http://trac.sagemath.org/ticket/18213#comment:25>
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 unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
