Opened 11 years ago

Closed 7 years ago

# Graphics3dGroup __add__ modifies its arguments

Reported by: Owned by: jason jason, was major sage-duplicate/invalid/wontfix graphics robertwb, mhampton, kcrisman, slabbe, ppurka Frédéric Chapoton N/A Infinite recursion in doctest

### Description

In an attempt to optimize, in some cases the add method of Graphics3dGroup modifies its arguments instead of returning a new Graphics3dGroup object. This breaks the user expectation, as illustrated below:

```a=point3d([1,0,0])+point3d([0,1,0])
b=point3d([0,0,1])
a # shows 2 points
a+b # shows all 3 points
a # Now this shows 3 points!!!
```

The attached patch deletes the offending optimization. If fast summing is needed, then the user can either create a Graphics3dGroup object themselves, or use something like `sage.misc.misc.balanced_sum`

### comment:1 Changed 11 years ago by jason

• Component changed from algebra to graphics
• Milestone set to sage-4.4.3
• Owner changed from AlexGhitza to jason, was

### comment:2 Changed 11 years ago by jason

• Status changed from new to needs_review

Credit goes to Ben Woodruff for reporting this issue.

### comment:3 Changed 11 years ago by robertwb

Do you have any stats on how much this affects performance?

### comment:4 Changed 11 years ago by jason

Before patch:

```sage: from sage.misc.misc import balanced_sum
sage: from sage.plot.plot3d.base import Graphics3dGroup
sage: lines=[line3d([[0,0,0],[cos(t),sin(t),1]]) for t in [0,0.05,..,6]]; len(lines)
121
sage: %timeit p=sum(lines)
625 loops, best of 3: 82.1 µs per loop
sage: lines=[line3d([[0,0,0],[cos(t),sin(t),1]]) for t in [0,0.05,..,6]]; len(lines)
121
sage: %timeit p=balanced_sum(lines)
625 loops, best of 3: 455 µs per loop
sage: lines=[line3d([[0,0,0],[cos(t),sin(t),1]]) for t in [0,0.05,..,6]]; len(lines)
121
sage: %timeit p=Graphics3dGroup(lines)
625 loops, best of 3: 179 µs per loop
```

After patch:

```sage: from sage.misc.misc import balanced_sum
sage: from sage.plot.plot3d.base import Graphics3dGroup
sage: lines=[line3d([[0,0,0],[cos(t),sin(t),1]]) for t in [0,0.05,..,6]]; len(lines)
121
sage: %timeit p=sum(lines)
625 loops, best of 3: 1.48 ms per loop
sage: lines=[line3d([[0,0,0],[cos(t),sin(t),1]]) for t in [0,0.05,..,6]]; len(lines)
121
sage: %timeit p=balanced_sum(lines)
625 loops, best of 3: 1.45 ms per loop
sage: lines=[line3d([[0,0,0],[cos(t),sin(t),1]]) for t in [0,0.05,..,6]]; len(lines)
121
sage: %timeit p=Graphics3dGroup(lines)
625 loops, best of 3: 180 µs per loop

```

So, as could be expected, performance of sum is impacted quite a bit. However, I would still say that the current behavior is wrong, and correctness trumps speed, especially if the overall total speed is still quite fast.

### comment:5 follow-up: ↓ 6 Changed 11 years ago by jason

Since we now have a Sage-written sum function, maybe we could just have the sum function call first_object._sum(list of things to sum) if it exists, and make a _sum method that does a Graphics3dGroup(sum_list) behind the scenes? This would also help with a recent ticket that ncohen opened about sum being really slow for linear programming stuff.

### comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 11 years ago by robertwb

Since we now have a Sage-written sum function, maybe we could just have the sum function call first_object._sum(list of things to sum) if it exists, and make a _sum method that does a Graphics3dGroup(sum_list) behind the scenes? This would also help with a recent ticket that ncohen opened about sum being really slow for linear programming stuff.

That's a great idea! (Certainly a new ticket.)

### comment:7 in reply to: ↑ 6 Changed 11 years ago by jason

Since we now have a Sage-written sum function, maybe we could just have the sum function call first_object._sum(list of things to sum) if it exists, and make a _sum method that does a Graphics3dGroup(sum_list) behind the scenes? This would also help with a recent ticket that ncohen opened about sum being really slow for linear programming stuff.

That's a great idea! (Certainly a new ticket.)

Actually, I just checked, and something like it is already being done. If you do sum(something,...), then if something has a sum method, it is called: something.sum(...). Of course, this won't work with lists or generators.

Anyways, feel free to review this ticket!

### comment:8 Changed 11 years ago by jason

Okay, the sum stuff is at #9115

### comment:9 Changed 11 years ago by jason

ping about reviewing this bug fix. There has been several times in the past few weeks that this bug has caught me.

### comment:10 Changed 11 years ago by kcrisman

• Reviewers set to Karl-Dieter Crisman
• Status changed from needs_review to positive_review

Looks ok.

```sage: len(G)
2
```

in the old example as desired, inheritance is correct. Currently building doc to make sure looks right... okay. Should `:meth:__add__` point to the method in `sage.plot.plot3d.base.Graphics3d`via hyperlink? Otherwise positive review, though of course the speed thing would be great to take care of if #9115 becomes available.

### comment:11 Changed 11 years ago by davidloeffler

• Status changed from positive_review to needs_work

I'm getting a doctest failure with this patch on 4.5.alpha1:

```**********************************************************************
File "/storage/masiao/sage-4.5.alpha1/devel/sage-reviewing/sage/plot/plot.py", line 428:
sage: g.show()
Exception raised:
Traceback (most recent call last):
File "/storage/masiao/sage-4.5.alpha1/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/storage/masiao/sage-4.5.alpha1/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/storage/masiao/sage-4.5.alpha1/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_3[11]>", line 1, in <module>
g.show()###line 428:
sage: g.show()
File "base.pyx", line 1081, in sage.plot.plot3d.base.Graphics3d.show (sage/plot/plot3d/base.c:10184)
File "base.pyx", line 524, in sage.plot.plot3d.base.Graphics3d.tachyon (sage/plot/plot3d/base.c:4785)
File "base.pyx", line 1408, in sage.plot.plot3d.base.Graphics3dGroup.texture_set (sage/plot/plot3d/base.c:13371)
File "base.pyx", line 1408, in sage.plot.plot3d.base.Graphics3dGroup.texture_set (sage/plot/plot3d/base.c:13371)
File "base.pyx", line 1408, in sage.plot.plot3d.base.Graphics3dGroup.texture_set (sage/plot/plot3d/base.c:13371)
File "base.pyx", line 1408, in sage.plot.plot3d.base.Graphics3dGroup.texture_set (sage/plot/plot3d/base.c:13387)
TypeError: reduce() of empty sequence with no initial value
**********************************************************************
```

Also, if I install this together with the patches at #9066 I get a bunch more failures coming in:

```sage -t  -long devel/sage/sage/plot/plot3d/shapes2.py # 5 doctests failed
```

### Changed 11 years ago by jason

apply on top of previous patches

### comment:12 Changed 11 years ago by jason

• Status changed from needs_work to needs_review

The errors should be taken care of now: on 4.5.2, plot/*.py* and plot/plot3d/*.py* all pass doctests. Apply the two patches on top of each other.

### comment:13 Changed 11 years ago by jason

Ping to someone to look at this. I believe I corrected all of the failing doctests.

### comment:14 Changed 11 years ago by davidloeffler

• Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, David Loeffler
• Status changed from needs_review to positive_review

Looks OK to me. I don't use the graphics code myself; but it had a positive review before I grumbled about failing doctests, and I can confirm that the doctests are now fixed, so I feel that merits giving it a positive review again.

### comment:15 Changed 11 years ago by jason

Thanks. This bug hit me again yesterday.

### comment:16 follow-up: ↓ 17 Changed 11 years ago by davidloeffler

• Status changed from positive_review to needs_work
• Work issues set to Infinite recursion in doctest

Hang on a minute. I was lazy and ran long doctests on graphics and only short doctests on everything else, so I missed a weird side-effect of this patch: if you install the two patches above on vanilla 4.6.alpha1 and do

```sage -t -long sage/combinat/root_system/weyl_group.py
```

then you get an infinite loop:

```File "/storage/masiao/sage-4.6.alpha1/devel/sage-hacking/sage/combinat/root_system/weyl_group.py", line 27:
sage: d.show3d(color_by_label=True, edge_size=0.01, vertex_size=0.03) #long time (less than one minute)
Exception raised:
Traceback (most recent call last):
File "/storage/masiao/sage-4.6.alpha1/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/storage/masiao/sage-4.6.alpha1/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/storage/masiao/sage-4.6.alpha1/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_0[7]>", line 1, in <module>
d.show3d(color_by_label=True, edge_size=RealNumber('0.01'), vertex_size=RealNumber('0.03')) #long time (less than one minute)###line 27:
sage: d.show3d(color_by_label=True, edge_size=0.01, vertex_size=0.03) #long time (less than one minute)
File "/storage/masiao/sage-4.6.alpha1/local/lib/python/site-packages/sage/graphs/generic_graph.py", line 12407, in show3d
color_by_label=color_by_label, **kwds).show()
File "base.pyx", line 1048, in sage.plot.plot3d.base.Graphics3d.show (sage/plot/plot3d/base.c:9726)
File "base.pyx", line 953, in sage.plot.plot3d.base.Graphics3d._process_viewing_options (sage/plot/plot3d/base.c:9519)
File "base.pyx", line 198, in sage.plot.plot3d.base.Graphics3d._determine_frame_aspect_ratio (sage/plot/plot3d/base.c:3307)
File "base.pyx", line 214, in sage.plot.plot3d.base.Graphics3d._safe_bounding_box (sage/plot/plot3d/base.c:3460)
File "base.pyx", line 1271, in sage.plot.plot3d.base.Graphics3dGroup.bounding_box (sage/plot/plot3d/base.c:12236)
File "base.pyx", line 1271, in sage.plot.plot3d.base.Graphics3dGroup.bounding_box (sage/plot/plot3d/base.c:12236)
File "base.pyx", line 1271, in sage.plot.plot3d.base.Graphics3dGroup.bounding_box (sage/plot/plot3d/base.c:12236)
File "base.pyx", line 1271, in sage.plot.plot3d.base.Graphics3dGroup.bounding_box (sage/plot/plot3d/base.c:12236)
File "base.pyx", line 1271, in sage.plot.plot3d.base.Graphics3dGroup.bounding_box (sage/plot/plot3d/base.c:12236)
File "base.pyx", line 1271, in sage.plot.plot3d.base.Graphics3dGroup.bounding_box (sage/plot/plot3d/base.c:12236)
File "base.pyx", line 1271, in sage.plot.plot3d.base.Graphics3dGroup.bounding_box (sage/plot/plot3d/base.c:12236)
[ ... ]
File "base.pyx", line 1271, in sage.plot.plot3d.base.Graphics3dGroup.bounding_box (sage/plot/plot3d/base.c:12236)
File "base.pyx", line 1271, in sage.plot.plot3d.base.Graphics3dGroup.bounding_box (sage/plot/plot3d/base.c:12236)
File "base.pyx", line 1271, in sage.plot.plot3d.base.Graphics3dGroup.bounding_box (sage/plot/plot3d/base.c:12236)
File "base.pyx", line 1271, in sage.plot.plot3d.base.Graphics3dGroup.bounding_box (sage/plot/plot3d/base.c:12236)
File "base.pyx", line 1271, in sage.plot.plot3d.base.Graphics3dGroup.bounding_box (sage/plot/plot3d/base.c:12234)
RuntimeError: maximum recursion depth exceeded in __subclasscheck__
**********************************************************************
1 of   8 in __main__.example_0
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/masiao/.sage//tmp/.doctest_weyl_group.py
[41.9 s]

----------------------------------------------------------------------
The following tests failed:

sage -t -long "devel/sage-hacking/sage/combinat/root_system/weyl_group.py"
Total time for all tests: 41.9 seconds
```

Apologies for my sloppiness in not having caught this bug earlier.

### comment:17 in reply to: ↑ 16 Changed 11 years ago by jason

Apologies for my sloppiness in not having caught this bug earlier.

Well, please accept my apologies for not thinking we had to check ptestlong. I'll fix it and run ptestlong before posting a new patch!

### comment:20 Changed 8 years ago by jdemeyer

• Milestone changed from sage-5.11 to sage-5.12

### comment:21 Changed 8 years ago by vbraun_spam

• Milestone changed from sage-6.1 to sage-6.2

### comment:22 Changed 7 years ago by vbraun_spam

• Milestone changed from sage-6.2 to sage-6.3

### comment:23 Changed 7 years ago by vbraun_spam

• Milestone changed from sage-6.3 to sage-6.4

### comment:24 Changed 7 years ago by chapoton

• Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
• Status changed from needs_work to needs_review

solved in #17258

### comment:25 Changed 7 years ago by kcrisman

• Authors Jason Grout deleted
• Reviewers changed from Karl-Dieter Crisman, David Loeffler to Frédéric Chapoton
• Status changed from needs_review to positive_review

### comment:26 Changed 7 years ago by vbraun

• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.