Add new option to hyperbolic graphics to select the model
sagetrac-jhonrubia6 opened this issue · 76 comments
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
Changed keywords from none to hyperbolic geometry plot
New commits:
82a041a | Added the disc model to hyperbolic_arc and polygons |
Commit: 82a041a
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 |
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.
thank you for your time reviewing the ticket. I'll work on this issues (one at a time)
Branch pushed to git repo; I updated commit sha1. New commits:
4851eed | HyperbolicArcCore object contains common methods to HyperbolicaArc and HyperbolicPolygon. Minor formatting done |
Branch pushed to git repo; I updated commit sha1. New commits:
bc476cd | The patch now includes the aforementioned changes |
This is a partial commit, it lacks your suggestion about a description of the different hyperbolic plane models yet.
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 |
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 |
Branch pushed to git repo; I updated commit sha1. New commits:
7199b8d | Merge branch 'develop' into t/22081/add_new_option_to_hyperbolic_graphics_to_select_the_model |
787cf58 | Merge branch 'u/jhonrubia6/add_new_option_to_hyperbolic_graphics_to_select_the_model' of git://trac.sagemath.org/sage into t/22081/add_new_option_to_hyperbolic_graphics_to_select_the_model |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
32f36ca | Added the disc model to hyperbolic_arc and polygons |
08eaab2 | HyperbolicArcCore object contains common methods to HyperbolicaArc and HyperbolicPolygon. Minor formatting done |
8222412 | The patch now includes the aforementioned changes |
0694eb6 | A simple explanation of both models (UHP and PD) included. Some references added |
Branch pushed to git repo; I updated commit sha1. New commits:
a7d1367 | Hyperbolic_regular_polygon class modified to be compatible with the new parameter of the base class HyperbolicPolygon.However it does not, as for this commit, perform hyperbolic_regular_polygon plots in the Poincare disc |
This feature was requested at
It would be nice to revive this ticket. What remains to be done?
There are some doctests to add for full coverage, some seemingly trivial doctest failures, and perhaps some other cleanup. I have rebased the branch and did a little cleanup. There is still a bit more work to do, but I don't think I have time right now to finish this.
New commits:
ee968f3 | Merge branch 'u/jhonrubia6/add_new_option_to_hyperbolic_graphics_to_select_the_model' of git://trac.sagemath.org/sage into public/graphics/hyperbolic_graphics-22081 |
Author: Javier Honrubia
Reviewer: Travis Scrimshaw
Setting a new milestone for this ticket based on a cursory review.
Changed author from Javier Honrubia to Javier Honrubia González
Fix author name to match name on other tickets.
Branch pushed to git repo; I updated commit sha1. New commits:
2375f2a | Merge branch 'develop' into t/22081/public/graphics/hyperbolic_graphics-22081 |
Poincaré disk model completed
I am now wondering a bit why we are not using (and possibly combining with) the code already provided by HyperbolicPlane for this. It seems to be doing the same computations and it would be good for code maintenance. I know some of the implementation details need to be different, but I suspect there is code to be shared, which would give the other models essentially for free.
I agree. As a matter of fact we had this same doubt in #22002 comment:3 five years ago. I see no trouble in relying on the geometry code, as its .plot()[0] are in fact <class 'sage.plot.arc.Arc'> and the same programming logic can be used to transform them in bezier arcs allowing for filling.
Is this a decision that we alone can make?
I thought I had already said something like this. hahaha I don't see any problem with making these design decisions; you are the one working on this code trying to improve it.
Ok. I'll work on this line of thinking, using HyperbolicPlane behind the scene
Branch pushed to git repo; I updated commit sha1. New commits:
70b8254 | Changed the approach to take advantage of the plotting function in geometry/hyperbolic_geodesic. Not fully tested yet, but sounds good |
Branch pushed to git repo; I updated commit sha1. New commits:
2ecc87f | added support for infinity vertices on UHP, added some plots. Some minor changes in hyperbolic_ploygon to support polygons with a vertex on infinity in UHP |
Branch pushed to git repo; I updated commit sha1. New commits:
8e80bc9 | Added a plot of an ideal triangle |
Branch pushed to git repo; I updated commit sha1. New commits:
1b46f13 | Merge branch 'develop' into t/22081/public/graphics/hyperbolic_graphics-22081 |
Let me know when this is fully ready for review.
Also, minor comment:
- A hyperbolic triangle with a vertex at Infinity
+ A hyperbolic triangle with a vertex at Infinity::Replying to @tscrim:
Let me know when this is fully ready for review.
I'll do. Right now it does work for PD and UHP using the geometry code. Next I will try to make it work for KM too.
Also, minor comment:
- A hyperbolic triangle with a vertex at Infinity + A hyperbolic triangle with a vertex at Infinity::
ok
Branch pushed to git repo; I updated commit sha1. New commits:
2fe7b35 | Added Klein Model to the hyperbolic_arc and hyperbolic_polygon |
Branch pushed to git repo; I updated commit sha1. New commits:
9b53e18 | Added partial implementation of Hiperboloid model. It does not support nor color (fairy easy) nor fill oprion: The latter may be subject for a new ticket in the future |
Branch pushed to git repo; I updated commit sha1. New commits:
09a5c9b | Added color and fill option to the hiperboloid model. The fill option is somewhat jaggy in some cases, but to this end I think its improvement should be deferred to a later ticket |
I also cleaned the code using
tox -e pycodestyle -- src/sage/plot/hyperbolic_polygon.py
tox -v -e relint src/sage/plot/hyperbolic_polygon.py
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. |
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. :)
Branch pushed to git repo; I updated commit sha1. New commits:
25c0d32 | Some last little doc tweaks and fixes. |
Doc looked good modulo a few more tweaks I did. I also fixed up some pyflakes, which modulo that the patchbot was green.
From the log, the one patchbot docbuild seems like it has an unrelated error. Everything else is green.
looks good to me. Thanks for your contribution. I learn new python tricks everyday :-)
Then positive review?
Branch pushed to git repo; I updated commit sha1. New commits:
3f367ec | Fixing one last typo hiper -> hyper. |
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
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.
Ok. I were not sure of how the 'etiquette' worked out in this case :-)
**********************************************************************
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
----------------------------------------------------------------------
Branch pushed to git repo; I updated commit sha1. New commits:
29f335c | Truncated doctest for numerical noise. |
This should take care of any numerical noise.
Changed branch from public/graphics/hyperbolic_graphics-22081 to 29f335c