Fix 3d plots to not ignore user prespecified aspect_ratio.

[nthiery]

In the following, the user specified aspect ratio is currently ignored:

    sage: p = sphere()
    sage: p.aspect_ratio([10,1,1])
    sage: p.show()

The attached patch is but a proof-of-concept of solution. I don't know well enough the plot code to see if this is the right thing to do.

---

Apply attachment: trac_14223-fix_3d_aspect_ratio.patch to devel/sage.

Component: graphics

Author: Punarbasu Purkayastha

Reviewer: Tobias Weich

Merged: sage-5.13.beta1

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

[nthiery]

Attachment: trac_14223-plot-aspect_ratio-nt.patch.gz

[ppurka]

comment:1

This is quite tricky. I think the modification to the aspect_ratio should be made much earlier since the scaling of the frame_aspect_ratio depends on the value of aspect_ratio. Will look into this.

[nthiery]

comment:2

Replying to @ppurka:

This is quite tricky. I think the modification to the aspect_ratio should be made much earlier since the scaling of the frame_aspect_ratio depends on the value of aspect_ratio. Will look into this.

Thanks!

[ppurka]

comment:3

Attached is the patch to fix this bug.

Patchbot apply trac_14223-fix_3d_aspect_ratio.patch

[ppurka]

Author: Punarbasu Purkayastha

[sagetrac-twch]

comment:5

Hi,

I wanted to test this patch but unfortunately I already failed applying the patch to the latest stable version (5.11). I get

sage: hg_sage.apply("/home/xyz/trac_14223-fix_3d_aspect_ratio.patch")
cd "/usr/local/sage-5.0.1/devel/sage" && sage --hg import   "/home/tobi/Sync/Projekte/SageDev_linestyle/review_aspect/trac_14223-fix_3d_aspect_ratio.patch"
applying /home/xyz/trac_14223-fix_3d_aspect_ratio.patch
abort: failed to synchronize metadata for "sage/plot/plot3d/base.pyx"

Being rather inexperienced it might well be that it is my mistake (especially as the patchbot seems to be ok with the patch). Do I have to apply it to the latest pre-release? Or do you have any other ideas what's wrong?

[ppurka]

comment:6

sage: hg_sage.apply("/home/xyz/trac_14223-fix_3d_aspect_ratio.patch")
cd "/usr/local/sage-5.0.1/devel/sage" && <snip>

There you go. It is trying to apply to sage-5.0.1 instead of sage-5.11.

[sagetrac-twch]

comment:7

No I don't think so. The folder name is missleading. I upgraded sage several times and the upgrades seem not to change the folder names.

On which version is the patch based on? I'll install the most recent pre-release and try whether I can apply it there...

[ppurka]

comment:8

IIRC, the patch is based on either sage-5.11-beta3 or sage-5.11-rc0. It should apply even to recent 5.12 prereleases since no one touches these graphics code. :-) The patchbot of course agrees!

[sagetrac-twch]

comment:9

strange... The problem persists even with the most recent version:

┌────────────────────────────────────────────────────────────────────┐                                                                                                                          
│ Sage Version 5.12.beta4, Release Date: 2013-08-30                  │                                                                                                                          
│ Type "notebook()" for the browser-based notebook interface.        │                                                                                                                                                                                                         
│ Type "help()" for help.                                            │                                                                                                                                                                                                         
└────────────────────────────────────────────────────────────────────┘                                                                                                                                                                                                         
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓                                                                                                                                                                                                         
┃ Warning: this is a prerelease version, and it may be unstable.     ┃                                                                                                                                                                                                         
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛                                                                                                                                                                                                         
sage: hg_sage.apply("/home/xyz/trac_14223-fix_3d_aspect_ratio.patch")                                                                                                                                                           
cd "/usr/local/sage-5.12.beta4/devel/sage" && sage --hg import   "/home/tobi/Sync/Projekte/SageDev_linestyle/review_aspect/trac_14223-fix_3d_aspect_ratio.patch"                                                                                                               
applying /home/xyz/trac_14223-fix_3d_aspect_ratio.patch                                                                                                                                                                         
abort: failed to synchronize metadata for "sage/plot/plot3d/base.pyx"                 

I somehow still suspect that the problem is on my side. Do you have any Ideas? Or could you check whether you can apply it to 5.12beta4?

Thanks,

Tobi

[ppurka]

comment:10

I can apply it to sage-5.12beta4. The aspect ratio comes out correct - see this plot.

Also, the patch applies correctly.

...age-5.12.beta4/devel/sage» ../../sage 
┌────────────────────────────────────────────────────────────────────┐
│ Sage Version 5.12.beta4, Release Date: 2013-08-30                  │
│ Type "notebook()" for the browser-based notebook interface.        │
│ Type "help()" for help.                                            │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: hg_sa
hg_sage    hg_sagenb  
sage: hg_sage.apply("https://github.com/sagemath/sage-prod/files/10657284/trac_14223-fix_3d_aspect_ratio.patch.gz")
Attempting to load remote file: https://github.com/sagemath/sage-prod/files/10657284/trac_14223-fix_3d_aspect_ratio.patch.gz
Loading: [.]
cd "/home/punarbasu/Installations/sage-5.12.beta4/devel/sage" && sage --hg import   "/home/punarbasu/.sage/temp/ub2/25836/tmp_uGrWYL.patch"
applying /home/punarbasu/.sage/temp/ub2/25836/tmp_uGrWYL.patch

[sagetrac-twch]

comment:11

ok, this is strange...:

After I downloaded the patch once more it applies perfectly to 5.11 and 5.12beta4. Probably there was just an error in the downloaded file. Thanks for your support and sorry that I didn't check this before, but I didn't expect something like this.

I'll now have a closer look at this patch!

[sagetrac-twch]

comment:12

Hi Basu,

here my remarks on the patch. It applies perfectly and in almost all cases which I tested it fixes the bug. I only found the following case, where setting the aspect ratio seems still to be ignored:

var('x,y')
P=plot3d(x^2-9*y^2,(x,-3,3),(y,-1,1))
P.aspect_ratio((1,1,1))
P.show()

is different from

var('x,y')
P=plot3d(x^2-9*y^2,(x,-3,3),(y,-1,1))
P.show(aspect_ratio=(1,1,1))

furthermore I observed that by this patch

P.show(aspect_ratio=(1,2,3))

changes the value of

P._aspect_ratio

and thus also the behaviour in all following show() commands. One can surely discuss if this is usefull or not, but I think in the 2d plots the setting of aspect ration in a show command only affects this plot and not the following. I think the 3d behavior should be consistent.

[ppurka]

comment:14

Thanks for finding out some bad examples. I will look into fixing these problems sometime next week.

[ppurka]

comment:15

I have tried to address both the concerns. The output should look the same now in all cases.

Patchbot apply trac_14223-fix_3d_aspect_ratio.patch

[sagetrac-twch]

comment:16

Hi!

Thanks for fixing these issues. All the problems which I reported above are indeed fixed.

I'm sorry to come up with another one:

The problem occurs if one has plots, with very different axes scales e.g.

var('x,y')
P=plot3d(x^2-100*y^2,(x,-10,10),(y,-1,1))

Without the patch the aspect ratio was automatically chosen such that all axes are equally long, if the user didn't set the aspect ratio. Now the aspect is automatically 1 which produces very degenerate plots, where one cannot see much. The user has thus to adjust the ration manually which seems a drawback to me.

As far as I understand in my limited view the problem is the following:

-the default value of self.aspect_ratio() is (1,1,1)
-the default show option for aspect ratio is 'automatic'
-before it has been ignored if self.aspect_ratio() has been set manually and if there was no aspect ratio argument in show(), now the show argument aspect_ratio='automatic' is ignored and instead self.aspect_ratio() is taken. I the user didn't change anything, this is however equal to (1,1,1) and produces a different result.

I'm not sure how to fix this, but here some ideas which might hopefully be helpful. Is it maybe possible, that the default value of self.aspect_ratio() is 'automatic'? Then there would be no degenerate plots if the user doesn't change anything.
But even then we would have a problem: If the user sets the aspect by
self.aspect_ratio((1,2,3))
and later wants a plot with the automatic aspect ratio
P.show(aspect_ratio='automatic')
then this would again be ignored. Wouldn't it accordingly be also necessary to set the SHOW_DEFAULT of aspect_ratio to None??? If the show option aspect_ratio is None, we replace it by self.aspect_ratio() which is be default "automatic". This would allow to distinguish between no user input and the input 'automatic' but still return the automatic aspect_ration if the user does nothing at all.

[ppurka]

comment:17

This is actually a side effect of fixing the problem you mentioned in comment:12. The frame aspect ratio was being set to (1, 1, 1) when aspect ratio was "automatic". This resulted in the discrepancy you pointed out. Now the frame aspect ratio is always computed from the aspect ratio as it should be.

I can revert the patch back to the previous case while making sure that aspect ratio is not stored when it is passed on to show().

[sagetrac-twch]

comment:18

Well if it is not easily possible to distinguish whether the aspect ration has been set by the user to (1,1,1) with {self.aspect_ratio((1,1,1)) or if it is still the default, then I think it is better that the default behavior is, that a reasonable aspect ratio is determined automatically as before. especially with function plots it happens quite often, that the axes have different scales and it is very unconfortable to figure out which aspect ratio is usefull if one just wants a fas glanc at the function.

[ppurka]

comment:19

Attachment: trac_14223-fix_3d_aspect_ratio.patch.gz

Ok. I updated the patch and also fixed this bug:

sage: show(cube(), frame_aspect_ratio=1)
...
TypeError: 'sage.rings.integer.Integer' object is not iterable

The inconsistency in comment:12 remains, but it can not be reliably fixed.
We can check for aspect_ratio=[1.0,1.0,1.0] in addition to
aspect_ratio='automatic', but then a small change in aspect_ratio, for
example, aspect_ratio=[1.1, 1.0, 1.0] will again lead to inconsistency in
the computation of frame_aspect_ratio.

Patchbot apply trac_14223-fix_3d_aspect_ratio.patch

[sagetrac-twch]

comment:20

Sorry it took some days until I was able to have a closer look.

Everything except the inconsistency you mention works fine now. Also some other test produced the expected outcome.

Additionally I was playing Patchbot on my system and tested against 5.12beta3:

-patch applies without errors

-source builds without errors

-patch doesn't provoke any errors for a full doctest

-reference builds without problems and the changes lead to reasonable output

So there seems no reason not to give a positive review...

[sagetrac-twch]

Reviewer: Tobias Weich

[jdemeyer]

Merged: sage-5.13.beta1