Add new option to hyperbolic graphics to select the model

[sagetrac-jhonrubia6]

As a first approach add the poincare disc model, and an option to select the model to use the drawing hyperbolic arcs and polygons

CC: @tscrim

Component: graphics

Keywords: hyperbolic geometry plot

Author: Javier Honrubia González

Branch/Commit: 29f335c

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/22081

[sagetrac-jhonrubia6]

Changed keywords from none to hyperbolic geometry plot

[sagetrac-jhonrubia6]

Branch: u/jhonrubia6/add_new_option_to_hyperbolic_graphics_to_select_the_model

[sagetrac-jhonrubia6]

New commits:

82a041a

Added the disc model to hyperbolic_arc and polygons

[sagetrac-jhonrubia6]

Commit: 82a041a

[sagetrac-git]

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

ea92d27

Merge branch 'develop' into t/22081/add_new_option_to_hyperbolic_graphics_to_select_the_model

[sagetrac-git]

Changed commit from 82a041a to ea92d27

[sagetrac-msaaltink]

comment:6

I like this new functionality. It would be great if regular polygons could also be drawn in the Poincare disc model.

Here are a few issues with the coding style:

In hyperbolic_arc.py, HyperbolicArc.init, the code

        else:
            if model == "PD":

would better be

        elif model == "PD":

which then requires less indenting for the following block.

HyperbolicArc.init should also report an error if model is neither 'UHP' or 'PD' rather than raising some other error. For example, hyperbolic_arc(0, 1, model='foo') raises an index error rather than reporting the actual user error.

The same two remarks apply to HyperbolicPolygon.init.

The style guide in PEP 8 says "Method definitions inside a class are surrounded by a single blank line.". Please do that here.

The coding guideline for docstrings says

It describes the function or method’s effect as a command (“Do this”, “Return that”), not as a description like “Returns the pathname ...”.

So for example, the documentation for _PD_hyperbolic_arc, "Function to construct an hyperbolic arc ...", should start "Construct an hyperbolic arc ...". The docstring for _bezier_path, "Returns the corresponding bezier path", should not start with "Returns the" and in any case should be more informative about the function. As this function does not in fact return anything, the docstring is quite misleading.

The docstrings should be more complete as well, for all method definitions, describing the inputs and output and containing at least one example or test. See http://doc.sagemath.org/html/en/developer/coding_basics.html#section-documentation-strings

There are several identical methods in hyperbolic_arc.py and hyperbolic_polygon.py. This code duplication is never a good idea if the functions are truly the same. Perhaps the two classes could inherit from some new class that contains a singly copy of these definitions---this will make repair easier if an error is discovered in future or some new functionality is to be added.

It would be useful somewhere to describe the different coordinate systems used for the two models, as it is not immediately obvious what are legitimate PD coordinates.

It is too bad that there is no model-independent way to specify the points (which would make it easy to plot the same diagram in the two different models). However, adding this probably goes beyond the scope of what you are trying to do here.

Finally, file /src/bin/sage-env-config should not be tracked.

[sagetrac-jhonrubia6]

comment:7

thank you for your time reviewing the ticket. I'll work on this issues (one at a time)

[sagetrac-git]

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

4851eed

HyperbolicArcCore object contains common methods to HyperbolicaArc and HyperbolicPolygon. Minor formatting done

[sagetrac-git]

Changed commit from ea92d27 to 4851eed

[sagetrac-git]

Changed commit from 4851eed to bc476cd

[sagetrac-git]

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

bc476cd

The patch now includes the aforementioned changes

[sagetrac-jhonrubia6]

comment:10

This is a partial commit, it lacks your suggestion about a description of the different hyperbolic plane models yet.

[sagetrac-git]

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

9d94d5c

A simple explanation of both models (UHP and PD) included. Some references added

[sagetrac-git]

Changed commit from bc476cd to 9d94d5c

[sagetrac-git]

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

aee6930

Merge branch 'develop' into t/22081/add_new_option_to_hyperbolic_graphics_to_select_the_model

[sagetrac-git]

Changed commit from 9d94d5c to aee6930

[sagetrac-git]

Changed commit from aee6930 to 787cf58

[sagetrac-git]

Changed commit from 09a5c9b to 900c36c

[sagetrac-git]

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

767722e	Merge branch 'public/graphics/hyperbolic_graphics-22081' of git://trac.sagemath.org/sage into public/graphics/hyperbolic_graphics-22081
900c36c	Reviewer changes simplifying plot implementation, more robust inputs, and misc doc changes.

[tscrim]

comment:42

Thank you for the additional changes. There were still a few bugs dealing with the different input types of CC versus vectors. I have tried to make the code more robust, but there are likely to be some other subtle bugs involved with an assumption of one type of input over the other. However, that can wait until another ticket.

I managed to streamline the arc construction with a little bit of refactoring and a well-placed getattr.

I made some other documentation tweaks and fixes.

If the bot comes back green (in particular, I think we should look at the doc output with the new tag above), then we can set a positive review if my changes are good.

Also, sometimes it can be good to ignore what the any linter/codestyle tool says in favor of readability and consistency with other code. :)

[sagetrac-git]

Changed commit from 900c36c to 25c0d32

[sagetrac-git]

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

25c0d32

Some last little doc tweaks and fixes.

[tscrim]

comment:44

Doc looked good modulo a few more tweaks I did. I also fixed up some pyflakes, which modulo that the patchbot was green.

[tscrim]

comment:45

From the log, the one patchbot docbuild seems like it has an unrelated error. Everything else is green.

[sagetrac-jhonrubia6]

comment:46

looks good to me. Thanks for your contribution. I learn new python tricks everyday :-)

[tscrim]

comment:47

Then positive review?

[sagetrac-git]

Changed commit from 25c0d32 to 3f367ec

[sagetrac-git]

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

3f367ec

Fixing one last typo hiper -> hyper.

[tscrim]

comment:49

Documentation did build prior to my trivial typo change.

[sagetrac-jhonrubia6]

comment:50

Replying to @tscrim:

Then positive review?

I do not understand. Are you asking me to change the status? :-)I don't mind changing it, if you prefer so

[tscrim]

comment:51

I have changed it, but since you were the one reviewing my changes (as I pushed them last), you are well within your rights to do so. :) Thank you.

[sagetrac-jhonrubia6]

comment:52

Ok. I were not sure of how the 'etiquette' worked out in this case :-)

[vbraun]

comment:53

**********************************************************************
File "src/sage/geometry/hyperbolic_space/hyperbolic_geodesic.py", line 2380, in sage.geometry.hyperbolic_space.hyperbolic_geodesic.HyperbolicGeodesicHM._plot_vertices
Failed example:
    g._plot_vertices(5)
Expected:
    [(4.0, -4.0, 5.744562646538029),
     (1.3632131724692114, -1.6370738298435326, 2.353372235315133),
     (0.13856858387448234, -0.9699800871213693, 1.4000223647674197),
     (-0.9425338542843988, -1.3076813974501533, 1.8969450977056184),
     (-3.0, -3.0, 4.358898943540652)]
Got:
    [(4.0, -4.0, 5.744562646538029),
     (1.3632131724692114, -1.6370738298435326, 2.353372235315132),
     (0.13856858387448057, -0.9699800871213675, 1.4000223647674161),
     (-0.9425338542843988, -1.3076813974501533, 1.8969450977056184),
     (-3.0, -3.0, 4.358898943540652)]
**********************************************************************
1 item had failures:
   1 of   6 in sage.geometry.hyperbolic_space.hyperbolic_geodesic.HyperbolicGeodesicHM._plot_vertices
    [447 tests, 1 failure, 7.70 s]
----------------------------------------------------------------------
sage -t --long --random-seed=0 src/sage/geometry/hyperbolic_space/hyperbolic_geodesic.py  # 1 doctest failed
----------------------------------------------------------------------

[sagetrac-git]

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

29f335c

Truncated doctest for numerical noise.

[sagetrac-git]

Changed commit from 3f367ec to 29f335c

[tscrim]

comment:55

This should take care of any numerical noise.

[vbraun]

Changed branch from public/graphics/hyperbolic_graphics-22081 to 29f335c