sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.37k stars 466 forks source link

Implement plots with logarithmic scale #4529

Closed 0d90674f-05eb-4b9d-9cb6-7d583bd8b339 closed 12 years ago

0d90674f-05eb-4b9d-9cb6-7d583bd8b339 commented 15 years ago

Attached is a patch which introduces log scale to Graphics() class.

Depends on #12974.

Apply


OLD DISCUSSION BELOW :)

Currently plot() has no option to use logarithmic scales.

One workaround is to use matplotlib directly, with its semilogy(), semilogx() and loglog() functions, but that wouldn't produce plots with the customisations implemented in sage. Another workaround is messing with the plot figure like:

import pylab
p=plot(x,marker='.')
f=pylab.figure()
f.gca().set_xscale('log')
p.save(figure=f)

But that creates two problems:

Also, this requires the user to know how to deal with figures, which is not directly exposed by sage.

There are some possibilities to fix that:

  1. Make plot() detect if the figure changes the scales and modify the adaptive algorithm and the axis codes accordingly
  2. Create a kwarg to tell plot() to implement the scale-change internally
  3. Create other functions to use loglog(), semilogx() and semilogy()
  4. Many (or all) of the above together, since they aren't mutually exclusive

From what I noticed, Mathematica implements the separate functions way, but it may be better to fix the issue in plot() itself and if the other functions are wanted, just make it so that they call plot() with the correct arguments

Depends on #12810 Depends on #12605 Depends on #12974

Component: graphics

Keywords: plot log scale

Author: Punarbasu Purkayastha, Karl-Dieter Crisman

Reviewer: Karl-Dieter Crisman, Punarbasu Purkayastha

Merged: sage-5.2.beta1

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

0d90674f-05eb-4b9d-9cb6-7d583bd8b339 commented 15 years ago

Attachment: sage0.png

"Wrong" sage plot with log scale

85eec1a4-3d04-4b4d-b711-d4db03337c41 commented 15 years ago
comment:1

This is a dupe if #4530

Cheers,

Michael

jasongrout commented 14 years ago
comment:5

See #1431 for a way to solve this.

jasongrout commented 13 years ago
comment:6

Since #1431 ended up just being about ticks (not the scale), #1431 doesn't directly address how to change the scale of the plot.

jasongrout commented 13 years ago
comment:7

Here are some comments about an API. How about adding a show keyword that specifies the scale of the axes.

scale='log' -- a string specifies the scale of the vertical axis

scale=('log','linear') -- a tuple or list of two strings specifies scales for both axes

We'd probably like some way to pass in arguments to the scale, since different scales have different options. This looks ugly: scale=( ('log', {'base': 2}), 'linear')

jasongrout commented 13 years ago
comment:8

We could also do something like xscale='log' or xscale=('log',{'base': 2}) and similarly for yscale. I don't like using x and y, though, since the variables in the plot might not be x and y.

kcrisman commented 13 years ago
comment:9

My sense is that the API should look like the tick marks API.

Here are some comments Jason made on sage-support about this.

> To change the scale, you can modify the plot afterwards, but I am 
> running into some sort of problem doing it: 
> sage: p=plot(e^x,(x,0,10)) 
> sage: m=p.matplotlib() 
> sage: from matplotlib.backends.backend_agg import FigureCanvasAgg 
> sage: m.set_canvas(FigureCanvasAgg(m)) 
> sage: m.gca().set_yscale('log') 
> sage: m.savefig('test.png') 

It seems something was wrong with the plot in the above example, or 
something.  Anyways, starting with: 
p=plot(x,(x,1,10)) 
works fine. 
To do #4529, I'd suggest adding a keyword to show that defines the 
scales of the x and y axes.  I've added some comments to the ticket. 

How was I not cc:ed on this ticket before? ;-)

kcrisman commented 13 years ago
comment:10

Also, #5128 would appear to be slightly related.

kcrisman commented 13 years ago
comment:11

The error with e^x is

MaskError: Cannot convert masked element to a Python int.

but seems to be related to there being something other than linearity involved. Linear functions work, anything with ^ or ** or sin doesn't.

--> 154         self._renderer.draw_text_image(font.get_image(), int(x), int(y) + 1, angle, gc)

is the problem - it's converting one of the elements, which is supposed to be skipped (masked, right?) to an int.

ppurka commented 12 years ago

plot in logarithmic scale.

ppurka commented 12 years ago
comment:12

Attachment: logplots.py.gz

attachment: logplots.py has a new class LogGraphics that I implemented and have been using for the past few months. Integrating it with Graphics seemed quite a painful process, so I had to go this direction and make my own class. Currently, it handles many but not all of the arguments that the Graphics class supports. In addition it uses matplotlib.plt to do the log plot; otherwise I ran into all sorts of problems with matplotlib (like the ones mentioned in earlier comments).

In engineering, we often need logarithmic plots and the logarithmic plots sometimes is of the form that the x-axis decreases as we go towards the right (for example if we plot decreasing probabilities on the x-axis). This LogGraphics takes this into account and makes sure that if a list of x-axis points with decreasing values along the higher indices of the list, then it plots the graph with a decreasing x-axis.

ppurka commented 12 years ago
comment:13

Sorry, I meant matplotlib.pyplot in the above comment.

ppurka commented 12 years ago
comment:15

I am not sure if this needs to be set to "needs_review". The main thing it is lacking is that it doesn't inherit the Graphics class, and hence the set of plot options it supports is much less.

On the other hand, I did try to make it inherit the Graphics class but then I ran into a big hurdle: the variables in the Graphics class are defined with double underscore __ and so even after I inherit it, I need to use (IMHO ugly) setters and getters in order to access those variables. I tried to overcome this limitation by inheriting Graphics in the class LogGraphics and defining a separate (and mostly identical) __init__ in LogGraphics but then the methods wouldn't work. Since I needed to rewrite almost everything, I decided to just rewrite everything from scratch.

One thing that I plan to do is change all the variables in the Graphics class to be defined with a single _ and see how it works out. Perhaps then it might be possible to integrate this patch better and consequently have access to all the methods (and hence plot options) available to the plot command.

kcrisman commented 12 years ago
comment:16

Hmm, that's odd that you had to do this. Here are two "Sage-ic" ideas.

ppurka commented 12 years ago
comment:17

That said, it is IMHO better to have logarithmic plots as a separate class. For instance, it doesn't make much sense to "add" plots with different scalings (and I also raise an error in the class I created).

kini commented 12 years ago
comment:18

I'm currently cleaning up tickets marked needs_review which have no patches attached, which includes this one, so back to needs_work this goes.

ppurka commented 12 years ago
comment:19

Replying to @kini:

I'm currently cleaning up tickets marked needs_review which have no patches attached, which includes this one, so back to needs_work this goes.

That's fine. I am actually working on a patch which

  1. modifies the Graphics class to have all the attributes start with a single underscore ._ instead of .__. This is already working and passes all doctects at least in devel/sage/sage/plot
  2. inherits the Graphics class and introduces logarithmic plots in a separate file. This is in progress and I hope I have a patch to attach to this ticket soon.
jasongrout commented 12 years ago
comment:20

Awesome. Just yesterday I had a feature request for log-log and semilog plots!

ppurka commented 12 years ago

Dependencies: 12974

ppurka commented 12 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,18 @@
+Attached is a patch which introduces log scale to `Graphics()` class.
+
+Apply the following patches in the specified order. `SAGE_ROOT` is the directory where the sage installation is present.
+
+```
+cd SAGE_ROOT/devel/sage
+../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10655552/trac_12974-fix_graphics_attributes_and_reorder_args.patch.gz
+../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10655554/trac_12974-refactor_and_whitespace_cleanups.patch.gz
+../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10642604/trac_4529-add_logscale_to_Graphics.patch.gz
+../../sage -b
+```
+
+---
+**OLD DISCUSSION BELOW :)**
+
 Currently plot() has no option to use logarithmic scales.

 One workaround is to use matplotlib directly, with its semilogy(), semilogx() and loglog() functions, but that wouldn't produce plots with the customisations implemented in sage.
@@ -25,3 +40,5 @@
 4. Many (or all) of the above together, since they aren't mutually exclusive

 From what I noticed, Mathematica implements the separate functions way, but it may be better to fix the issue in plot() itself and if the other functions are wanted, just make it so that they call plot() with the correct arguments
+
+
ppurka commented 12 years ago
comment:21

I added a patch to Graphics class which introduces log plots. Some salient points

  1. I had to "disable" some tick formatting for log plots because matplotlib wasn't behaving well with the formatting that is done in Graphics().maptplotlib() (ex. the error in comment:11, out of memory error, etc)
  2. The patch in this ticket relies on the patches in #12974 which is mostly a cleanup of the Graphics class.
  3. In trying to implement my own class, I started to look at each of the matplotlib functions more carefully, and found out the reason(s) why setting the scale wasn't working (see point 1.). The result is that I could implement log scale right inside Graphics by carefully weeding out the corner cases. I hope I got all the corner cases.

Todo:

  1. A patch to plot() and other functions will take more time to implement. :(
  2. Probably need to make sure that user does not specify tick formatters and locators which don't behave well with log plots.
  3. Feedback is welcome! I need to know if I missed something.

Example code:

p = plot(exp, 1, 10)
p.set_scale('loglog')
p.show()
xd=range(-5,5); yd=[10**_ for _ in xd]; p=list_plot(zip(xd, yd),plotjoined=True)
p.set_yscale('log', 2) # Set only y-axis to log and with base of log being 2.
p.show()
ppurka commented 12 years ago
comment:22

Hmm.. there is still a problem if I modify the Graphics class. It becomes impossible to add 2D and 3D graphics.

ppurka commented 12 years ago
comment:23

Replying to @ppurka:

Hmm.. there is still a problem if I modify the Graphics class. It becomes impossible to add 2D and 3D graphics.

It was a silly thing. I just needed to reorder the check for ._*scale to after the check for Graphics3d in __add__(). The updated patch now passes all doctests in sage/plot! Also, SHOW_OPTIONS, matplotlib() have two extra arguments: scale, base which are identical in behavior to the arguments in set_scale(). So, now it is possible to do this:

p = plot(exp, 1, 10)
p.show(scale=('loglog', 2))
ppurka commented 12 years ago

Changed dependencies from 12974 to #12974

ppurka commented 12 years ago

Apply to devel/sage

ppurka commented 12 years ago

Attachment: trac_4529-add_logscale_to_Graphics.patch.gz

Attachment: trac_4529-add_docs_eg_to_some_user_facing_functions.patch.gz

Apply to devel/sage

ppurka commented 12 years ago

Description changed:

--- 
+++ 
@@ -7,6 +7,7 @@
 ../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10655552/trac_12974-fix_graphics_attributes_and_reorder_args.patch.gz
 ../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10655554/trac_12974-refactor_and_whitespace_cleanups.patch.gz
 ../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10642604/trac_4529-add_logscale_to_Graphics.patch.gz
+../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10642605/trac_4529-add_docs_eg_to_some_user_facing_functions.patch.gz
 ../../sage -b
ppurka commented 12 years ago
comment:25

Added another patch to other user facing functions. Actually most of the functions in sage.plot.* work when scale=... is passed as an argument while calling the function. Using the log scale makes sense in only a couple of them though, and I have added some documentation and examples for the cases I think are pertinent.

Finally, I added some extra functions {loglog,semilogx,semilogy}_plot, and the corresponding list_plots. I think this should undergo a review now.

These set of patches passes all doctests in devel/sage/sage/plot.

kcrisman commented 12 years ago

Author: Punarbasu Purkayastha

kcrisman commented 12 years ago
comment:26

Patchbot: Apply attachment: trac_4529-add_logscale_to_Graphics.patch and attachment: trac_4529-add_docs_eg_to_some_user_facing_functions.patch.

kcrisman commented 12 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,8 @@
 Attached is a patch which introduces log scale to `Graphics()` class.

-Apply the following patches in the specified order. `SAGE_ROOT` is the directory where the sage installation is present.
+Depends on #12974.  Apply [attachment: trac_4529-add_logscale_to_Graphics.patch](https://github.com/sagemath/sage-prod/files/10642604/trac_4529-add_logscale_to_Graphics.patch.gz) and [attachment: trac_4529-add_docs_eg_to_some_user_facing_functions.patch](https://github.com/sagemath/sage-prod/files/10642605/trac_4529-add_docs_eg_to_some_user_facing_functions.patch.gz).
+
+Alternately, apply the following patches in the specified order. `SAGE_ROOT` is the directory where the sage installation is present.

cd SAGE_ROOT/devel/sage

kcrisman commented 12 years ago
comment:27

I hope to be able to go over this very valuable idea at the current Bug Days. Trivial comment while I'm doing a cursory read-through

- ``linear`` -- both the axes are linear. 

should probably indicate

- ``'linear'`` -- both the axes are linear. 

or

- 'linear' -- both the axes are linear. 

and similarly in all other cases, especially when looking at inputs (since it's very important that these are strings, not just commands.


Comments:

ppurka commented 12 years ago
comment:28

It just seemed easier and cleaner to handle the scaling by hardcoding the scale. I will see how the patch turns out if I don't hardcode it.

Most people will expect the functions semilog* and loglog* to be present. For instance, matlab has all of those commands (matlab has it only for list plots, the function plotter called ezplot does not have it AFAIK), and mathematica has LogLogPlot and LogLinearPlot.

If it is not desirable, then we can simply have just two functions log_plot(scale='loglog'|'semilogx'|'semilogy', funcs, ...), and log_list_plot(scale='loglog'|'semilogx'|'semilogy', data, ...).

ppurka commented 12 years ago

Work Issues: doctests, strings

jasongrout commented 12 years ago
comment:29

+1 to having loglog and semilog convenience functions.

kcrisman commented 12 years ago
comment:30

I think that anything that roughly approximates Mathematica and Matlab is fine, it was just having so many of them that might be a problem. log_plot and semilog_plot(axis=foo) or something better would be good, and perhaps a couple similar list_plot ones... I have to admit I always just use points() instead of list_plot, is list_plot used as the name in one of these programs?

Jason, how many would be useful? It's the semilogx and semilogy that seems a bit much, though if it's standard in other programs I guess it would be ok.

jasongrout commented 12 years ago
comment:31

I personally would say a loglog and semilog (defaulting to semilogy) would be good, with an option to switch the semilog to x or y. I guess a list plot would be convenient too, though I agree with you that points() or line() in general should be used over list_plot. They are more powerful (mostly) anyway.

ppurka commented 12 years ago
comment:32

Replying to @jasongrout:

I personally would say a loglog and semilog (defaulting to semilogy) would be good, with an option to switch the semilog to x or y. I guess a list plot would be convenient too, though I agree with you that points() or line() in general should be used over list_plot. They are more powerful (mostly) anyway.

For consistency, we should have just one convention. It is very confusing if the options of plot (except for probably plotjoined and data) are also valid options for list_plot, but then we introduce an inconsistency via log plots. So, I would be in favor of either

  1. Don't have any of the loglog_*, semilog* and handle scaling only through the scale and base parameters of plot and list_plot (and actually all other plots)
  2. Have all the functions loglog_plot, loglog_list_plot available, and perhaps change semilog[xy]* to semilog* with an extra optional argument log_axis='x'/'y'. In case we follow this second rule, I would like this extra argument to be different from axis because it can be confused with axes=True/False.

I would really like this issue to be sorted out first.

kcrisman commented 12 years ago
comment:33

For consistency, we should have just one convention.

Agreed.

It is very confusing if the options of plot (except for probably plotjoined and data) are also valid options for list_plot, but then we introduce an inconsistency via log plots. So, I would be in favor of either

How would this introduce an inconsistency? Is the suggestion on the table that the log option would only be for one of them? I don't see why we can't have our cake and eat it too.

  1. Don't have any of the loglog_*, semilog* and handle scaling only through the scale and base parameters of plot and list_plot (and actually all other plots)

If Mma and friends have it, this is probably not a good idea.

  1. Have all the functions loglog_plot, loglog_list_plot available, and perhaps change semilog[xy]* to semilog* with an extra optional argument log_axis='x'/'y'. In case we follow this second rule, I would like this extra argument to be different from axis because it can be confused with axes=True/False.

Yes, that's a very good idea!

I would really like this issue to be sorted out first.

Agreed. Jason, should we raise this on sage-devel?

jasongrout commented 12 years ago
comment:34

Sure, let's raise it on sage-devel. Make sure the proposal provides specific options to vote for.

kcrisman commented 12 years ago
comment:35

Replying to @jasongrout:

Sure, let's raise it on sage-devel. Make sure the proposal provides specific options to vote for.

Okay, hope I did it clearly enough.

http://groups.google.com/group/sage-devel/browse_thread/thread/af20ea19c09d14a0

ppurka commented 12 years ago
comment:36

Thanks kcrisman. That poll is comprehensive enough.

Updated the Graphics patch. This now has modifications only to matplotlib and sister functions, and leaves the Graphics class's attributes alone.

There was a problem with the ticker in that the labels were in scientific notation (ex. 1e10) and not in the base^exponent form (ex. 10^10). This is now fixed, except for the case when the user enters a custom tick formatter. This last case is up to the user to handle.

The interface to plot and list_plot remains unchanged. However, I will wait for the poll in sage-devel before deciding what extra plot commands to introduce.

ppurka commented 12 years ago

Description changed:

--- 
+++ 
@@ -1,15 +1,16 @@
 Attached is a patch which introduces log scale to `Graphics()` class.

-Depends on #12974.  Apply [attachment: trac_4529-add_logscale_to_Graphics.patch](https://github.com/sagemath/sage-prod/files/10642604/trac_4529-add_logscale_to_Graphics.patch.gz) and [attachment: trac_4529-add_docs_eg_to_some_user_facing_functions.patch](https://github.com/sagemath/sage-prod/files/10642605/trac_4529-add_docs_eg_to_some_user_facing_functions.patch.gz).
+Depends on #12974.

-Alternately, apply the following patches in the specified order. `SAGE_ROOT` is the directory where the sage installation is present.
+Apply the following patches in the specified order. `SAGE_ROOT` is the directory where the sage installation is present.

cd SAGE_ROOT/devel/sage -../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10655552/trac_12974-fix_graphics_attributes_and_reorder_args.patch.gz -../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10655554/trac_12974-refactor_and_whitespace_cleanups.patch.gz -../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10642604/trac_4529-add_logscale_to_Graphics.patch.gz -../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10642605/trac_4529-add_docs_eg_to_some_user_facing_functions.patch.gz +../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10655553/trac_12974-fix_graphics_attributes.patch.gz +../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10655555/trac_12974-refactor.patch.gz +../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10655556/trac_12974-reorder_some_arguments.patch.gz +../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10655557/trac_12974-whitespace_cleanup.patch.gz +../../sage -hg qimport -P https://github.com/sagemath/sage-prod/files/10642607/trac_4529-add_logscale_to_Graphics.2.patch.gz ../../sage -b

ppurka commented 12 years ago

Changed work issues from doctests, strings to convenience functions

ppurka commented 12 years ago
comment:38

Updated to the correct patch. Apparently I uploaded the wrong patch several hours ago.

kcrisman commented 12 years ago

Reviewer: Karl-Dieter Crisman

kcrisman commented 12 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,10 @@
 Attached is a patch which introduces log scale to `Graphics()` class.

 Depends on #12974.
+
+Apply [attachment: trac_4529-add_logscale_to_Graphics.2.patch](https://github.com/sagemath/sage-prod/files/10642607/trac_4529-add_logscale_to_Graphics.2.patch.gz) and a patch to be determined.
+
+OR

 Apply the following patches in the specified order. `SAGE_ROOT` is the directory where the sage installation is present.
kcrisman commented 12 years ago
comment:39

Some comments:

       if scale is None: 
            return ('linear', 'linear', 10, 10) 

could return 'linear', 'linear', None, None?

sage: G = plot(exp(x), (x,5,10))
sage: G.show(scale=('semilogy', 2))

I don't even think this is a very atypical example to arise in practice. It should be documented somehow.

But even with all of these comments, and waiting for the post-poll patch, fantastic job on this. Someone had to come along to finally wrap this for us, it's been requested zillions of times, and this is very worth the effort, thank you so much.

ppurka commented 12 years ago
comment:40

Replying to @kcrisman:

Some comments:

  • Shouldn't base=2 raise an error when scale='linear' in your example? Maybe the
       if scale is None: 
            return ('linear', 'linear', 10, 10) 

could return 'linear', 'linear', None, None?

I had thought about it. My decision was to silently ignore this error because it is not fatal in any way and we handle it properly (i.e. we ignore it and do the right thing).

Edit: This seems to be the same behavior as in matplotlib.

  • In _matplotlib_tick_formatter, should base and scale be next to each other in the function definition? (This is a very minor critique, of course.)

Well, except for subplot, the rest of the arguments are alphabetically arranged. :) Personally, I find it quite hard to find out where a particular function or argument is present in a typical Sage code. There is no particular manner in which the functions are arranged. Especially in several thousand line files like graphics.py it becomes hard to scroll around and edit code.

  • Regardless of the outcome of the poll (on which you can vote), I think one should add a lot more examples in the documentation for show for the various options. Lots of them.

I will add some more.

  • What's going on with the pr, i = *, 0 thing removed? I just don't know what it had been doing - seems to have been dead code, but I always get nervous when I have no idea what it *used'' to do...

Yes. I have no idea what it was for. It is dead code, so I removed it.

  • kini says that the [13:] seems brittle if matplotlib's API changes; would it be possible to remove the specific string \\mathdefault instead?

To remove it from matplotlib, we need to set rcParams['text.usetex']=True. But this makes matplotlib try to compile latex on its own and use dvipng to convert from dvi to png, etc. Moreover, this parameter seems to be persistent and remains throughout the current session. So, simply editing the string seemed a more viable option to me.

If the API changes (which seems unlikely to me), then the fix will be very easy too.

  • I wonder about the not setting of the spines outward when the axes shouldn't cross. Here is an example which serves the point:
sage: G = plot(exp(x), (x,5,10))
sage: G.show(scale=('semilogy', 2))

I don't even think this is a very atypical example to arise in practice. It should be documented somehow.

I will have to see how to handle this. Messing around with the spines was one of the primary reasons why setting scale wasn't working - the "converting masked to int" error.

  • It's fairly easy to have just one tick in a given direction, which usually raises an error in normal plots but isn't raising an error for yours. I'm not sure if one would want to raise an error like "Use a different base so that you get at least two ticks!" or something.

I think it is up to the user to either change their range, or their base, or provide custom ticks.

But even with all of these comments, and waiting for the post-poll patch, fantastic job on this. Someone had to come along to finally wrap this for us, it's been requested zillions of times, and this is very worth the effort, thank you so much.

Thanks. I needed it for my own research! :)

kini commented 12 years ago
comment:41

Oh, I didn't mean to prevent \\mathdefault from coming into the string at all. I meant to just specifically remove the substring \\mathdefault (say with .replace("\\mathdefault","") or something).