Opened 8 years ago

Closed 7 years ago

#14223 closed defect (fixed)

Fix 3d plots to not ignore user prespecified aspect_ratio.

Reported by: nthiery Owned by: jason, was
Priority: major Milestone: sage-5.13
Component: graphics Keywords:
Cc: Merged in: sage-5.13.beta1
Authors: Punarbasu Purkayastha Reviewers: Tobias Weich
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

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 trac_14223-fix_3d_aspect_ratio.patch to devel/sage.

Attachments (2)

trac_14223-plot-aspect_ratio-nt.patch (2.5 KB) - added by nthiery 8 years ago.
trac_14223-fix_3d_aspect_ratio.patch (5.5 KB) - added by ppurka 7 years ago.

Download all attachments as: .zip

Change History (25)

Changed 8 years ago by nthiery

comment:1 follow-up: Changed 8 years ago by 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.

comment:2 in reply to: ↑ 1 Changed 8 years ago by nthiery

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!

comment:3 Changed 7 years ago by ppurka

  • Authors set to Punarbasu Purkayastha
  • Description modified (diff)
  • Milestone changed from sage-5.11 to sage-5.12
  • Status changed from new to needs_review

Attached is the patch to fix this bug.

Patchbot apply trac_14223-fix_3d_aspect_ratio.patch

comment:4 Changed 7 years ago by twch

comment:5 Changed 7 years ago by twch

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?

comment:6 Changed 7 years ago by ppurka

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.

comment:7 Changed 7 years ago by twch

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

comment:8 Changed 7 years ago by ppurka

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!

comment:9 Changed 7 years ago by twch

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

comment:10 Changed 7 years ago by ppurka

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("http://trac.sagemath.org/raw-attachment/ticket/14223/trac_14223-fix_3d_aspect_ratio.patch")
Attempting to load remote file: http://trac.sagemath.org/raw-attachment/ticket/14223/trac_14223-fix_3d_aspect_ratio.patch
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

comment:11 Changed 7 years ago by twch

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!

comment:12 Changed 7 years ago by twch

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.

comment:13 Changed 7 years ago by twch

  • Status changed from needs_review to needs_work

comment:14 Changed 7 years ago by ppurka

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

comment:15 Changed 7 years ago by ppurka

  • Status changed from needs_work to needs_review

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

comment:16 Changed 7 years ago by twch

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.

comment:17 Changed 7 years ago by ppurka

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

comment:18 Changed 7 years ago by twch

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.

Changed 7 years ago by ppurka

comment:19 Changed 7 years ago by ppurka

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

comment:20 Changed 7 years ago by twch

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

comment:21 Changed 7 years ago by twch

  • Reviewers set to Tobias Weich
  • Status changed from needs_review to positive_review

comment:22 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:23 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.13.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.