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 )
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)
Change History (25)
Changed 8 years ago by
comment:1 follow-up: ↓ 2 Changed 8 years ago by
comment:2 in reply to: ↑ 1 Changed 8 years ago by
Replying to ppurka:
This is quite tricky. I think the modification to the
aspect_ratio
should be made much earlier since the scaling of theframe_aspect_ratio
depends on the value ofaspect_ratio
. Will look into this.
Thanks!
comment:3 Changed 7 years ago by
- 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
comment:5 Changed 7 years ago by
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
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
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
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
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
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
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
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
- Status changed from needs_review to needs_work
comment:14 Changed 7 years ago by
Thanks for finding out some bad examples. I will look into fixing these problems sometime next week.
comment:15 Changed 7 years ago by
- 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
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
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
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
comment:19 Changed 7 years ago by
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
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
- Reviewers set to Tobias Weich
- Status changed from needs_review to positive_review
comment:22 Changed 7 years ago by
- Description modified (diff)
comment:23 Changed 7 years ago by
- Merged in set to sage-5.13.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
This is quite tricky. I think the modification to the
aspect_ratio
should be made much earlier since the scaling of theframe_aspect_ratio
depends on the value ofaspect_ratio
. Will look into this.