Plotting the Mandelbrot set in Sage

948efc99-6194-4ac3-9373-d64154b28c08 commented 7 years ago

This ticket is the first in a series of tickets that will be opened this summer in an effort to improve the functionality for complex dynamics in Sage. I have added complex_dynamics folder to the dynamics folder that introduces the function mandelbrot_plot() which allows users to produce an interactive plot of the Mandelbrot set. For more information on my Google Summer of Code Project you can visit the following link: https://benbarros.wordpress.com/

CC: @sagetrac-atowsley @kcrisman

Component: dynamics

Keywords: gsoc2017, complexdynamics

Author: Ben Barros

Branch/Commit: f52fb3f

Reviewer: Ben Hutz

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

948efc99-6194-4ac3-9373-d64154b28c08 commented 7 years ago

Branch: u/bbarros/23257

948efc99-6194-4ac3-9373-d64154b28c08 commented 7 years ago

Commit: 023321f

948efc99-6194-4ac3-9373-d64154b28c08 commented 7 years ago

New commits:

023321f Added complex_dynamics folder for plotting Mandelbrot set

bhutz commented 7 years ago

Reviewer: Ben Hutz

bhutz commented 7 years ago

comment:3

First, some general things:

don't include the helper file in the documentation (rst)
also do not add the pyx extension since the helper functions aren't exposed to the user
I'd add to the ticket description that this is the first of a series improving the complex dynamics functionality.

for the .py file

OUTPUT: - isn't this a .bmp or an interact?
kwds go under an INPUT: section
add some readability spacing, such as after , for arguments

kwds.pop("x_center", -1.0)

I don't understand the section 'NOTEBOOK EXAMPLES:'. Seems like this is all self-explanatory from the kwds description. The actual function call examples are needed, but they should be in an EXAMPLES: section
I would add some to more the general function description:
- what is the mandelbrot set
- how are you computing it (perhaps a very basic ALGORITHM: section)

for the .pyx file

I don't think you need the initial underscore (that marks it as a private function that won't appear in tab completion) since it is not exposed to the user anyway.
need one line description of function
why not a cdef?
OUTPUT - isn't this a .bmp
The examples fail because you need to import the function first

sage: from sage.dynamics.complex_dynamics.mandel_julia_helper import _fast_mandel_plot

bug: negative image_width flips the image instead of gives an error

a couple functionality thoughts:

for interact:

I'd let the user adjust pretty much anything
I still don't really like the width slider. How about a input box?

for colors: the contour levels are not great by default. I was playing around with it and the best I could come up with using basically the same system was as follows

have two parameters

number of iterations between levels (default to 1)
number of colors to use

you'll need to do better with your nested ifs. For example

cdef int level
if iteration != max_iteration:
    level = iteration/level_sep

if level < color_num:
    pixel[row,col] = color_list[level]
else:
    pixel[row,col] = color_list[-1]

This gave me pretty good control over the contours and I got a nice grey-scale image with base_color [50,50,50] with level_sep=1 and 20 colors gradations. This has the advantage of disengaging it from the max_iterations. So you can increase the detail without messing up the contours.

I'll leave the default color choice to you, but at least consider gray scale since it should allow a 'white' external ray to show up well.

bhutz commented 7 years ago

Changed keywords from GSoC, complex, dynamics to gsoc2017, complexdynamics

948efc99-6194-4ac3-9373-d64154b28c08 commented 7 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-I have added complex_dynamics folder to the dynamics folder that introduces the function mandelbrot_plot() which allows users to produce an interactive plot of the Mandelbrot set.
+This ticket is the first in a series of tickets that will be opened this summer in an effort to improve the functionality for complex dynamics in Sage. I have added complex_dynamics folder to the dynamics folder that introduces the function mandelbrot_plot() which allows users to produce an interactive plot of the Mandelbrot set.

948efc99-6194-4ac3-9373-d64154b28c08 commented 7 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-This ticket is the first in a series of tickets that will be opened this summer in an effort to improve the functionality for complex dynamics in Sage. I have added complex_dynamics folder to the dynamics folder that introduces the function mandelbrot_plot() which allows users to produce an interactive plot of the Mandelbrot set.
+This ticket is the first in a series of tickets that will be opened this summer in an effort to improve the functionality for complex dynamics in Sage. I have added complex_dynamics folder to the dynamics folder that introduces the function mandelbrot_plot() which allows users to produce an interactive plot of the Mandelbrot set. For more information on my Google Summer of Code Project you can visit the following link: [https://benbarros.wordpress.com/](https://benbarros.wordpress.com/)

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

dbcd279 Updated and debugged complex_dynamics files

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 023321f to dbcd279

948efc99-6194-4ac3-9373-d64154b28c08 commented 7 years ago

comment:7

I pushed an updated branch with the changes made. I did not change the function in the .pyx file to a cdef because whenever I try to import it as a cdef (using or cimport or just import) I am unable to import it. If you have any ideas of how to get that to work I would love to hear them. Please let me know what else I need to fix.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from dbcd279 to bcb3792

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

bcb3792 23257: Fixed syntax and doctests

bhutz commented 7 years ago

comment:9

I think the functionality works well now. I have a few code nitpicks and one suggestion for speedup.

You also need to resolve the current merge conflict

cython file:

import statements to top

line 89 too long

line 98: don't over do the spacing: 2new_xnew_y + y_coor

add some comments describing what each code block is doing. for example, why are you reflecting about the x-axis. Are you trying to use that it is symmetrical? In that case you should be assigning two pixel values for each iteration and only looping through the top half of the pixels, but only when the real axis is contain in the image window.

speed ups: the quantities

image_width (row-pixel_count / 2) / pixel_count image_width (col-pixel_count / 2) / pixel_count

are better off precomputed as step values

python file

reference should go in the main reference file: src/doc/en/reference/references/index.rst

then you can reference it here as [Devaney]_

you also have several lines longer than the 80char standard. They should be wrapped.

several examples are missing the 'sage:' portion

imports to top of file

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

6a9f953 23257: Fixed documentation and syntax

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from bcb3792 to 6a9f953

kcrisman commented 7 years ago

comment:12

If you are doing this sort of thing, maybe eventually it will supersede e.g. #8423, #1004, #11837 ? See also this old sage-devel thread.

kcrisman commented 7 years ago

comment:13

Various comments I hope are useful:

This branch doesn't apply to latest develop, apparently?
Separately, it would be useful to know whether the interactive functionality works anywhere than in the SageNB notebook. Jupyter? SMC sagews? Sage Cell Server?
Also, one could probably use the newish Sphinx plot directive thingamabob to put a few in the documentation...
Does this plot Julia sets, as the file seems to indicate? What about plot for anything other than the square+add map? (E.g. z^3+c.)
How long does testing these take? You may need to mark some tests # long time.

I understand some of this may be answered by your work later in the GSOC summer but wanted to try to get that all out there. Good luck - as you can tell from some of my references in comment:12, this has been missing in Sage for a LONG time.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

`f74fac8`	`Merge branch 'master' into 23257`
`e1a3791`	`23257: Changed to lazy_import in complex_dynamics/all.py`
`1afa332`	`23257: Fixed merge conflict and doctests`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 6a9f953 to 1afa332

948efc99-6194-4ac3-9373-d64154b28c08 commented 7 years ago

comment:15

Thanks for the feedback! The plotting of the Julia set is what we plan to work on next after finishing up everything with the Mandelbrot set so we thought we should name the functions accordingly. I have not tried to use the interact functionality in anywhere besides the Sage notebook so I will be sure to try those out and make a note of it in the documentation. I am also not familiar with Sphinx so I will have to take a look at that. As far as a more general version of the function, we plan to get to that after we tackle Julia sets.

Thanks again,

Ben Barros

Replying to @kcrisman:

Various comments I hope are useful:

This branch doesn't apply to latest develop, apparently?

Separately, it would be useful to know whether the interactive functionality works anywhere than in the SageNB notebook. Jupyter? SMC sagews? Sage Cell Server?

Also, one could probably use the newish Sphinx plot directive thingamabob to put a few in the documentation...

Does this plot Julia sets, as the file seems to indicate? What about plot for anything other than the square+add map? (E.g. z^3+c.)

How long does testing these take? You may need to mark some tests # long time.

I understand some of this may be answered by your work later in the GSOC summer but wanted to try to get that all out there. Good luck - as you can tell from some of my references in comment:12, this has been missing in Sage for a LONG time.

948efc99-6194-4ac3-9373-d64154b28c08 commented 7 years ago

comment:16

I added a TESTS block in the documentation of mandelbrot_plot and changed all of the examples to have the flag # not tested. I don't quite like the look of this in the documentation but it was the only I way I could think of for now. Here is an example of what I am talking about:

sage: mandelbrot_plot() # not tested

sage: mandelbrot_plot(pixel_count=1000) # not tested

sage: mandelbrot_plot(base_color=[70, 40, 240]) # not tested

sage: mandelbrot_plot(x_center=-0.75, y_center=0.25, image_width=1/2, number_of_colors=75) # not tested

"To use the function outside of the notebook, you must set interact to False"

sage: mandelbrot_plot(interact=False) # not tested
Launched png viewer for 500x500px 24-bit RGB image

sage: mandelbrot_plot(interact=False, x_center=-1.11, y_center=0.2283, image_width=1/128, # not tested
....: max_iteration=2000, number_of_colors=500, base_color=[40, 100, 100])
Launched png viewer for 500x500px 24-bit RGB image

bhutz commented 7 years ago

comment:18

An easy fix: lighter not darker for more iterations

I understand preventing the huge html ones from running in the EXAMPLES so that the user isn't bombarded. It is good that they are there so the user can see how to call the function with various parameters. But why are the non-interact ones blocked as well? It seems that you just add them later in TESTS section. I'd just put them in the example section and let them run.

Do you still need to add the 'Extension' for importing the .pyx? Since those are now private functions, I didn't think you needed that anymore.

I can't say I like the interact html tests. Maybe post a question to sage-devel about how to test an interact with a doctest, or even if you should be trying to.

kcrisman commented 7 years ago

comment:19

I can't say I like the interact html tests. Maybe post a question to sage-devel about how to test an interact with a doctest, or even if you should be trying to.

Don't bother trying to actually test them. BUT you should definitely still include this as documentation (not TESTS) so that people know what the heck to do; the point is to get it used.

I highly recommend creating a CoCalc account and trying this with the Jupyter (or even locally) and .sagews format. You'd need some kind of internet access or special permission from William to try out your branch, but that should be doable. I don't see any reason it shouldn't work though if you are using the same widgets.

jdemeyer commented 7 years ago

comment:20

For example about how to include and test interacts, see src/sage/interacts/library.py.
There is a specific widget for colors, use that instead of an input box.
Don't show() the plot, but return the plot.
In the Cython code: why use float which has limited precision? I would guess that things get more interesting with double precision, especially for deeper zooms. Second, it's less important, but I would use long instead of int. It would be unlikely that somebody would use more than 2³¹ pixels or iterations, but at least we should not a priori exclude it either.
In the Cython code: use sig_check to make the code interruptible, see http://cysignals.readthedocs.io/en/latest/interrupt.html
In the Cython code: better add typing for every variable like x_step.
To be compatible with Python 3: add fromfutureimport absolute_import, division at the top of your files and consider whether you want floor division (use // in that case) or true division.

jdemeyer commented 7 years ago

comment:21

Obviously, do test mandelbrot_plot() (I guess the reason that you don't is number 3 from my last comment).
The formatting of TESTS is wrong, it should be TESTS:: with indentation.

kcrisman commented 7 years ago

comment:22

Okay, this is pretty impressive. But still some stuff for sure needs to be done. Jeroen has some very good ideas for Cython.

I'm not sure whether the default should be interactive or not. Usually one would make the static one the default.

Second, it's not clear to me that the only way one wants to interact with this is via typing in things. Sliders make much more sense for at least generic zooming exploration. I don't know whether this should be an option or show people how to see them ...

jdemeyer commented 7 years ago

comment:23

I just saw that there is an existing Mandelbrot interact implementation interacts.fractals.mandelbrot() in src/sage/interacts/library.py. You should at least try that one and maybe see if you can reuse something or replace that one with your version...

kcrisman commented 7 years ago

comment:24

Ha, I totally forgot about that one! Indeed,

from .library import mandelbrot, julia, cellular_automaton

Hopefully the work you did here is good background for something far more general ...

948efc99-6194-4ac3-9373-d64154b28c08 commented 7 years ago

comment:25

The plan is to get the Mandelbrot set plotted and then move on to more general functionality such as plotting external rays on the Mandelbrot set, plotting of Julia sets, and eventually plotting the Mandelbrot set for more general functions.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

9b6738b 23257: Cython, Python 3 improvements, Color widget added

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 1afa332 to 9b6738b

948efc99-6194-4ac3-9373-d64154b28c08 commented 7 years ago

comment:27

I cleaned up the documentation, added sig_check() to make the function interruptable, Python 3 division, changed the floats to doubles, and ints to longs. I have been playing around with sliders for a while and it isn't very easy to get the precision required when zooming in. They may be convenient for navigating around the image but they are usually too sensitive when you zoom in far enough. I plan to replace the code in the interacts library eventually. However, the function there is slightly more general than mine right now (it plots the set for different exponents of z) so I will leave it until I can replace it with a more general version.

kcrisman commented 7 years ago

comment:29

I have been playing around with sliders for a while and it isn't very easy to get the precision required when zooming in. They may be convenient for navigating around the image but they are usually too sensitive when you zoom in far enough.

Oh, totally. However, it's also very hard to navigate using just the typed-in numbers. Maybe there can be some combination of that ... I wonder. Anyway, something to think about.

I plan to replace the code in the interacts library eventually.

I assume yours ends up being faster and/or more accurate? That will be welcome.

videlec commented 7 years ago

comment:30

Why is it called fast_mandel_plot and not fast_mandelbrot_plot?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

a1d02e4 23257: Changed fast_mandel_plot to fast_mandelbrot_plot

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 9b6738b to a1d02e4

948efc99-6194-4ac3-9373-d64154b28c08 commented 7 years ago

comment:32

Replying to @videlec:

Why is it called fast_mandel_plot and not fast_mandelbrot_plot?

Good question. I never really thought about that. I could change that pretty easily though.

New commits:

a1d02e4 23257: Changed fast_mandel_plot to fast_mandelbrot_plot

bhutz commented 7 years ago

comment:33

This is looking good, I just have a couple minor nitpicks.

in the INPUT
- you have f(z) in max_iteration instead of Q_c(z) (both files)
- interact to false is not just for outside the notebook(in the .py file)
I thought you were going to change the default for interact to False
line 78 of .pyx : initialize misspelled
It took me a little bit to figure out why your y_step uses -y_center. Then I realized that it isn't really the y_step, that is scale_factor. Rather x_step, y_step represent the upper left hand corner of the image. And scale_factor really is the step_size. Then you are using the x-axis symmetry of the image. Any reason, you are not just using the correct y coordinate? Why don't you just use

y_step = y_center + image_width/2
y_coor = y_step - col*scale_factor

I also suggest renaming those variables to match their purpose and making a comment that you are starting at the upper left hand corner of the image.
line 102: sig_check not needed (you already check in the inside loop)

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from a1d02e4 to 8aae69c

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

8aae69c 23257: Changed default for interact to False

948efc99-6194-4ac3-9373-d64154b28c08 commented 7 years ago

comment:35

I changed the default for the interact to False and consequently changed the examples a little bit. I also renamed and cleaned up the variables dealing with the coordinates. Please let me know if there's anything else that catches your attention.

bhutz commented 7 years ago

comment:36

Went to build the docs from scratch and there is an error:

[dochtml] [dynamics ] /home/ben/sage/sage-test/src/doc/en/reference/dynamics/complex_dynamics.rst:: WARNING: document isn't included in any toctree
[dochtml] Error building the documentation.

While we're renaming variables. Aren't your 'row' and 'col' variable as reversed.

Everything else looks fine

kcrisman commented 7 years ago

comment:37

To be fair, that could be an unrelated thing due to switching between branches; try make doc-clean and then make the doc again? See #22588.

bhutz commented 7 years ago

comment:38

It was a clean doc build.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

f52fb3f 23257: Added complex_dynamics to dynamics/index.rst

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 8aae69c to f52fb3f

bhutz commented 7 years ago

comment:41

Since 8.0.rc0 I out, I assume this gets pushed to 8.1 now.

vbraun commented 7 years ago

Changed branch from u/bbarros/23257 to f52fb3f

sagemath / sage

Plotting the Mandelbrot set in Sage #23257