Opened 3 months ago

Closed 8 weeks ago

#23257 closed enhancement (fixed)

Plotting the Mandelbrot set in Sage

Reported by: bbarros Owned by:
Priority: minor Milestone: sage-8.1
Component: dynamics Keywords: gsoc2017, complexdynamics
Cc: atowsley, kcrisman Merged in:
Authors: Ben Barros Reviewers: Ben Hutz
Report Upstream: N/A Work issues:
Branch: f52fb3f (Commits) Commit: f52fb3fc9b32f6c24bdf3f308740603933d44600
Dependencies: Stopgaps:

Description (last modified by bbarros)

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/

Change History (43)

comment:1 Changed 3 months ago by bbarros

  • Branch set to u/bbarros/23257

comment:2 Changed 3 months ago by bbarros

  • Commit set to 023321fa6b77e9f749c0fb020bd85c41d9ca8858
  • Status changed from new to needs_review

New commits:

023321fAdded complex_dynamics folder for plotting Mandelbrot set

comment:3 Changed 3 months ago by bhutz

  • Cc atowsley added
  • Keywords gsoc2017 complexdynamics added; GSoC complex dynamics removed
  • Priority changed from major to minor
  • Reviewers set to Ben Hutz
  • Status changed from needs_review to needs_work

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.

comment:4 Changed 3 months ago by bbarros

  • Description modified (diff)

comment:5 Changed 3 months ago by bbarros

  • Description modified (diff)

comment:6 Changed 3 months ago by git

  • Commit changed from 023321fa6b77e9f749c0fb020bd85c41d9ca8858 to dbcd279521f5e5a76258981f4607bd4b4ad4ec90

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

dbcd279Updated and debugged complex_dynamics files

comment:7 Changed 3 months ago by bbarros

  • Status changed from needs_work to needs_review

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.

comment:8 Changed 3 months ago by git

  • Commit changed from dbcd279521f5e5a76258981f4607bd4b4ad4ec90 to bcb3792080656f66a79b3c88b8b8680380db185c

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

bcb379223257: Fixed syntax and doctests

comment:9 Changed 3 months ago by bhutz

  • Status changed from needs_review to needs_work

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: 2*new_x*new_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

comment:10 Changed 3 months ago by git

  • Commit changed from bcb3792080656f66a79b3c88b8b8680380db185c to 6a9f9531f02139f0d2d94c68dcd495902c8ac4e6

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

6a9f95323257: Fixed documentation and syntax

comment:11 Changed 3 months ago by kcrisman

  • Cc kcrisman added

comment:12 Changed 3 months ago by kcrisman

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.

comment:13 follow-up: Changed 3 months ago by 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. z3+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.

comment:14 Changed 3 months ago by git

  • Commit changed from 6a9f9531f02139f0d2d94c68dcd495902c8ac4e6 to 1afa33239d14c19261da3f710a7849532019f16b

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

f74fac8Merge branch 'master' into 23257
e1a379123257: Changed to lazy_import in complex_dynamics/all.py
1afa33223257: Fixed merge conflict and doctests

comment:15 in reply to: ↑ 13 Changed 3 months ago by bbarros

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. z3+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.

comment:16 Changed 3 months ago by bbarros

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

comment:17 Changed 3 months ago by bbarros

  • Status changed from needs_work to needs_review

comment:18 follow-up: Changed 3 months ago by bhutz

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.

comment:19 in reply to: ↑ 18 Changed 3 months ago by kcrisman

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.

comment:20 Changed 3 months ago by jdemeyer

  • Status changed from needs_review to needs_work
  1. For example about how to include and test interacts, see src/sage/interacts/library.py.
  1. There is a specific widget for colors, use that instead of an input box.
  1. Don't show() the plot, but return the plot.
  1. 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 231 pixels or iterations, but at least we should not a priori exclude it either.
  1. In the Cython code: use sig_check to make the code interruptible, see http://cysignals.readthedocs.io/en/latest/interrupt.html
  1. In the Cython code: better add typing for every variable like x_step.
  1. To be compatible with Python 3: add from __future__ import absolute_import, division at the top of your files and consider whether you want floor division (use // in that case) or true division.
Last edited 3 months ago by jdemeyer (previous) (diff)

comment:21 Changed 3 months ago by jdemeyer

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

comment:22 Changed 3 months ago by kcrisman

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 ...

comment:23 Changed 3 months ago by jdemeyer

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...

comment:24 Changed 3 months ago by kcrisman

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 ...

comment:25 Changed 3 months ago by bbarros

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.

comment:26 Changed 3 months ago by git

  • Commit changed from 1afa33239d14c19261da3f710a7849532019f16b to 9b6738b06bcfa199a4f7148a3cde5218fdb5f012

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

9b6738b23257: Cython, Python 3 improvements, Color widget added

comment:27 Changed 3 months ago by bbarros

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.

comment:28 Changed 3 months ago by bbarros

  • Status changed from needs_work to needs_review

comment:29 Changed 3 months ago by kcrisman

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.

comment:30 follow-up: Changed 3 months ago by vdelecroix

Why is it called fast_mandel_plot and not fast_mandelbrot_plot?

comment:31 Changed 3 months ago by git

  • Commit changed from 9b6738b06bcfa199a4f7148a3cde5218fdb5f012 to a1d02e4c35099cfe7bc344fd8647b38b526eacbb

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

a1d02e423257: Changed fast_mandel_plot to fast_mandelbrot_plot

comment:32 in reply to: ↑ 30 Changed 3 months ago by bbarros

Replying to vdelecroix:

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:

a1d02e423257: Changed fast_mandel_plot to fast_mandelbrot_plot

comment:33 Changed 3 months ago by bhutz

  • Status changed from needs_review to needs_work

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)

comment:34 Changed 3 months ago by git

  • Commit changed from a1d02e4c35099cfe7bc344fd8647b38b526eacbb to 8aae69cc30ad0e566e0e1fe6f7646822b91df50c

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

8aae69c23257: Changed default for interact to False

comment:35 Changed 3 months ago by bbarros

  • Status changed from needs_work to needs_review

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.

comment:36 Changed 3 months ago by bhutz

  • Status changed from needs_review to needs_work

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

comment:37 Changed 3 months ago by kcrisman

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.

comment:38 Changed 3 months ago by bhutz

It was a clean doc build.

comment:39 Changed 3 months ago by git

  • Commit changed from 8aae69cc30ad0e566e0e1fe6f7646822b91df50c to f52fb3fc9b32f6c24bdf3f308740603933d44600

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

f52fb3f23257: Added complex_dynamics to dynamics/index.rst

comment:40 Changed 3 months ago by bbarros

  • Status changed from needs_work to needs_review

comment:41 Changed 3 months ago by bhutz

  • Milestone changed from sage-8.0 to sage-8.1
  • Status changed from needs_review to positive_review

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

comment:42 Changed 2 months ago by bhutz

  • Component changed from fractals to dynamics

comment:43 Changed 8 weeks ago by vbraun

  • Branch changed from u/bbarros/23257 to f52fb3fc9b32f6c24bdf3f308740603933d44600
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.