#9439: hyperbolic geometry
------------------------------------------------------------------------------+
Reporter: vdelecroix |
Owner: vdelecroix
Type: enhancement |
Status: new
Priority: major |
Milestone: sage-5.11
Component: geometry |
Resolution:
Keywords: hyperbolic geometry, Poincare disc, upper half plane, sd35 |
Work issues:
Report Upstream: N/A |
Reviewers: Johan Bosman
Authors: Vincent Delecroix, Martin Raum |
Merged in:
Dependencies: |
Stopgaps:
------------------------------------------------------------------------------+
Comment (by vdelecroix):
Hi Greg,
There are many nice features in the patch but I did not start to play
with. You should start by a big clean:
1) there should be no trailing whitespace
2) the organization of each file should be a header which consists of a
string with authors the copyright statement and then the code. There
should not be several headers and copyright statements.
3) all methods and functions should be commented and doctested. Running
`sage -coverage` gives
{{{
SCORE hyperbolic_object.py: 10.0% (1 of 10)
Missing documentation:
* line 40: def to_model(self, model)
* line 49: def to_UHP(self)
* line 52: def uhp_representation(self)
* line 55: def model_representation(self)
* line 58: def representation_in_model(self, model=None)
* line 96: def to_model(self, model, **options)
* line 108: def graphics_options(self)
Missing doctests:
* line 33: def __init__(self, args, **graphics_options)
* line 88: def __init__(self, args, **graphics_options)
}}}
4) the files should be properly included in the sage documentation
Now, more serious issues
5) the objects from your module must be lazy imported and not imported in
the global namespace (in order to not slow down sage startup)
6) the method `show` should not return a graphics object but should rather
shows it! You could rename it `plot`.
7) I agree that it is misleading but `CC` in Sage is not the set of
complex number! In particular the test `x in CC` does not answer to the
question "does my object `x` modelizes some complex number ?". Precisely
`CC` is the set of floating point complex number with 53 bits of
precision. But is it on purpose that you want `x in CC` as coordinates for
a point in the upper half plane ?
8) The string representation "Hyperbolic point whose representation in the
Upper Half Plane is +Infinity" is definitely too long! Why not "Point in
HH +Infinity".
And finally a design question (which perhaps should be thought of first)
9) I think that the fact of being conformal or bounded etc is not a
property of a HyperbolicObject but rather a property of the underlying
model. You decided to not design a class for each model, was it on
purpose? Such a class may contain all these property. As you see, in my
initial patch, it was possible to write
{{{
sage: HH
Hyperbolic plane
sage: HH(0)
Boundary point 0
}}}
I would prefer to have a dedicated class for each model and be able to
construct point/geodesic/polygons from the class (via for example
``HH.point(data)``, ``HH.geodesic(data)``, etc). You may also move the
`random_element` methods and the various conversions between the models
into this parent class.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/9439#comment:24>
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/groups/opt_out.
