archive/issues_020706.json:
{
"assignees": [],
"body": "<div id=\"comment:0\"></div>\n\nThe method `_flip_c` currently uses\n\n```\nnr_ref = len(W.long_element(as_word=True))\n```\nwhich drastically slows down computations (here: the construction of cluster complexes using subword complexes through the greedy flip algorithm).\n\nCC: @tscrim @fchapoton @nthiery\n\nComponent: **combinatorics**\n\nKeywords: **reflection group, coxeter group, subword complex, days80**\n\nAuthor: **Christian Stump**\n\nBranch/Commit: **[`6f2172b`](https://github.com/sagemath/sagetrac-mirror/commit/6f2172b794ec53d55bd7920125b92662dc17edc8)**\n\nReviewer: **Fr\u00e9d\u00e9ric Chapoton**\n\n_Issue created by migration from https://trac.sagemath.org/ticket/20943_\n\n",
"closed_at": "2016-07-08T07:09:49Z",
"created_at": "2016-07-05T18:33:21Z",
"labels": [
"https://github.com/sagemath/sage/labels/c%3A%20combinatorics",
"https://github.com/sagemath/sage/labels/p%3A%20major%20/%203",
"https://github.com/sagemath/sage/labels/enhancement"
],
"milestone": "https://github.com/sagemath/sage/milestones/sage-7.3",
"reactions": [],
"repository": "https://github.com/sagemath/sage",
"title": "Update a missing important speed improvement for subword complexes",
"type": "issue",
"updated_at": "2016-07-08T07:09:49Z",
"url": "https://github.com/sagemath/sage/issues/20943",
"user": "https://github.com/stumpc5"
}The method _flip_c currently uses
nr_ref = len(W.long_element(as_word=True))
which drastically slows down computations (here: the construction of cluster complexes using subword complexes through the greedy flip algorithm).
CC: @tscrim @fchapoton @nthiery
Component: combinatorics
Keywords: reflection group, coxeter group, subword complex, days80
Author: Christian Stump
Branch/Commit: 6f2172b
Reviewer: Frédéric Chapoton
Issue created by migration from https://trac.sagemath.org/ticket/20943
archive/issue_events_292098.json:
{
"actor": "https://github.com/stumpc5",
"created_at": "2016-07-05T18:33:21Z",
"event": "milestoned",
"issue": "https://github.com/sagemath/sage/issues/20943",
"milestone_number": null,
"milestone_title": "sage-7.3",
"type": "issue_event",
"url": "https://github.com/sagemath/sage/issues/20943#event-292098"
}archive/issue_events_292099.json:
{
"actor": "https://github.com/stumpc5",
"created_at": "2016-07-05T18:33:21Z",
"event": "labeled",
"issue": "https://github.com/sagemath/sage/issues/20943",
"label": "https://github.com/sagemath/sage/labels/c%3A%20combinatorics",
"label_color": "0000ff",
"label_name": "c: combinatorics",
"label_text_color": "ffffff",
"type": "issue_event",
"url": "https://github.com/sagemath/sage/issues/20943#event-292099"
}archive/issue_events_292100.json:
{
"actor": "https://github.com/stumpc5",
"created_at": "2016-07-05T18:33:21Z",
"event": "labeled",
"issue": "https://github.com/sagemath/sage/issues/20943",
"label": "https://github.com/sagemath/sage/labels/p%3A%20major%20/%203",
"label_color": "ffbb00",
"label_name": "p: major / 3",
"label_text_color": "ffffff",
"type": "issue_event",
"url": "https://github.com/sagemath/sage/issues/20943#event-292100"
}archive/issue_events_292101.json:
{
"actor": "https://github.com/stumpc5",
"created_at": "2016-07-05T18:33:21Z",
"event": "labeled",
"issue": "https://github.com/sagemath/sage/issues/20943",
"label": "https://github.com/sagemath/sage/labels/enhancement",
"label_color": "696969",
"label_name": "enhancement",
"label_text_color": "ffffff",
"type": "issue_event",
"url": "https://github.com/sagemath/sage/issues/20943#event-292101"
}archive/issue_comments_304304.json:
{
"body": "<div id=\"comment:1\" align=\"right\">comment:1</div>\n\nI will provide code in a minute; any suggestions how to properly do get the number of reflections in the fastest way for `WeylGroups` or `CoxeterGroups`? Currently, it uses the definition of the degrees, but that is too slow given that it should and can be fast.",
"created_at": "2016-07-05T18:35:13Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304304",
"user": "https://github.com/stumpc5"
}comment:1
I will provide code in a minute; any suggestions how to properly do get the number of reflections in the fastest way for WeylGroups or CoxeterGroups? Currently, it uses the definition of the degrees, but that is too slow given that it should and can be fast.
archive/issue_comments_304305.json:
{
"body": "<div id=\"comment:2\" align=\"right\">comment:2</div>\n\nFor Weyl groups, I get\n\n```\nsage: W = WeylGroup(['E',7])\nsage: %timeit len(W.long_element(as_word=True))\n10 loops, best of 3: 98.6 ms per loop\nsage: %timeit W.number_of_reflections()\n1 loop, best of 3: 208 ms per loop\n```\nand for Coxeter groups, I get\n\n```\nsage: W = CoxeterGroup(['E',7])\nsage: %timeit len(W.long_element(as_word=True))\n1 loop, best of 3: 206 ms per loop\nsage: %timeit W.number_of_reflections()\n1 loop, best of 3: 378 ms per loop\n```\nso I leave the current implementation for those (though this is not as clean as it should be).",
"created_at": "2016-07-05T18:44:34Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304305",
"user": "https://github.com/stumpc5"
}comment:2
For Weyl groups, I get
sage: W = WeylGroup(['E',7])
sage: %timeit len(W.long_element(as_word=True))
10 loops, best of 3: 98.6 ms per loop
sage: %timeit W.number_of_reflections()
1 loop, best of 3: 208 ms per loop
and for Coxeter groups, I get
sage: W = CoxeterGroup(['E',7])
sage: %timeit len(W.long_element(as_word=True))
1 loop, best of 3: 206 ms per loop
sage: %timeit W.number_of_reflections()
1 loop, best of 3: 378 ms per loop
so I leave the current implementation for those (though this is not as clean as it should be).
archive/issue_comments_304306.json:
{
"body": "Branch: **[u/stumpc5/20943](https://github.com/sagemath/sagetrac-mirror/tree/u/stumpc5/20943)**",
"created_at": "2016-07-05T18:56:35Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304306",
"user": "https://github.com/stumpc5"
}Branch: u/stumpc5/20943
archive/issue_comments_304307.json:
{
"body": "<div id=\"comment:4\" align=\"right\">comment:4</div>\n\n\n```\nsage: W = ReflectionGroup(['A',6]); c = list(W.index_set())\nsage: Q = c+W.w0.coxeter_sorting_word(c)\nsage: %time S = SubwordComplex(Q,W.w0,algorithm=\"greedy\")\n```\nwith the fix, I get\n\n```\nCPU times: user 222 ms, sys: 16.1 ms, total: 238 ms\nWall time: 248 ms\n```\nand without the fix, I get\n\n```\nCPU times: user 1.35 s, sys: 37.7 ms, total: 1.39 s\nWall time: 1.4 s\n```\nFor types E, it becomes obviously much worse.",
"created_at": "2016-07-05T18:58:22Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304307",
"user": "https://github.com/stumpc5"
}comment:4
sage: W = ReflectionGroup(['A',6]); c = list(W.index_set())
sage: Q = c+W.w0.coxeter_sorting_word(c)
sage: %time S = SubwordComplex(Q,W.w0,algorithm="greedy")
with the fix, I get
CPU times: user 222 ms, sys: 16.1 ms, total: 238 ms
Wall time: 248 ms
and without the fix, I get
CPU times: user 1.35 s, sys: 37.7 ms, total: 1.39 s
Wall time: 1.4 s
For types E, it becomes obviously much worse.
archive/issue_comments_304308.json:
{
"body": "<div id=\"comment:5\" align=\"right\">comment:5</div>\n\nI tried to push and to set the branch by hand, but I am not sure it actually worked...",
"created_at": "2016-07-05T19:16:35Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304308",
"user": "https://github.com/stumpc5"
}comment:5
I tried to push and to set the branch by hand, but I am not sure it actually worked...
archive/issue_comments_304309.json:
{
"body": "Description changed:\n``````diff\n--- \n+++ \n@@ -1,4 +1,4 @@\n-The method ``_flip_c`` currently uses\n+The method `_flip_c` currently uses\n \n ```\n nr_ref = len(W.long_element(as_word=True))\n``````\n",
"created_at": "2016-07-05T19:16:35Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304309",
"user": "https://github.com/stumpc5"
}Description changed:
---
+++
@@ -1,4 +1,4 @@
-The method ``_flip_c`` currently uses
+The method `_flip_c` currently uses
```
nr_ref = len(W.long_element(as_word=True))archive/issue_events_292102.json:
{
"actor": "https://github.com/stumpc5",
"created_at": "2016-07-05T19:17:39Z",
"event": "labeled",
"issue": "https://github.com/sagemath/sage/issues/20943",
"label": "https://github.com/sagemath/sage/labels/needs%20review",
"label_color": "7fff00",
"label_name": "needs review",
"label_text_color": "ffffff",
"type": "issue_event",
"url": "https://github.com/sagemath/sage/issues/20943#event-292102"
}archive/issue_comments_304310.json:
{
"body": "<div id=\"comment:7\" align=\"right\">comment:7</div>\n\nSorry, but I do not understand the logic of your change.\n\nWhat is this attribute `__number_of_reflections` ? Does it exist only in the complex reflection group implementation ?\n\nOne should also replace the default code for `number_of_reflections` by the faster\n\n```\nfrom sage.rings.all import ZZ\nreturn ZZ.sum(self.degrees() - 1)\n```\nthat avoid to use rank.\n\n---\nNew commits:\n<table><tr><td><a href=\"https://github.com/sagemath/sagetrac-mirror/commit/dad4de4b43bb5405f3a2103ced4fc91cef5e9d65\"><code>dad4de4</code></a></td><td><code>fixed the bux</code></td></tr><tr><td><a href=\"https://github.com/sagemath/sagetrac-mirror/commit/aaf771ed669652a1b09d69b5e7c539afa950120a\"><code>aaf771e</code></a></td><td><code>Merge branch 'develop' into u/stumpc5/20943</code></td></tr></table>\n",
"created_at": "2016-07-06T09:55:28Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304310",
"user": "https://github.com/fchapoton"
}comment:7
Sorry, but I do not understand the logic of your change.
What is this attribute __number_of_reflections ? Does it exist only in the complex reflection group implementation ?
One should also replace the default code for number_of_reflections by the faster
from sage.rings.all import ZZ
return ZZ.sum(self.degrees() - 1)
that avoid to use rank.
New commits:
dad4de4 | fixed the bux |
aaf771e | Merge branch 'develop' into u/stumpc5/20943 |
archive/issue_comments_304311.json:
{
"body": "Commit: **[`aaf771e`](https://github.com/sagemath/sagetrac-mirror/commit/aaf771ed669652a1b09d69b5e7c539afa950120a)**",
"created_at": "2016-07-06T09:55:28Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304311",
"user": "https://github.com/fchapoton"
}Commit: aaf771e
archive/issue_comments_304312.json:
{
"body": "<div id=\"comment:8\" align=\"right\">comment:8</div>\n\nthe doctests are missing the `# optional`",
"created_at": "2016-07-06T10:05:43Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304312",
"user": "https://github.com/fchapoton"
}comment:8
the doctests are missing the # optional
archive/issue_comments_304313.json:
{
"body": "<div id=\"comment:9\" align=\"right\">comment:9</div>\n\nOkay, I guess you are right. I should and will do it properly:\n\n1. make the method `ReflectionGroup.number_of_reflections` use the attribute `ReflectionGroup._number_of_reflections` instead of recomputing it,\n\n2. make this flip code use the method instead of the attribute for `ReflectionGroup` and also use this attribute for `WeylGroup` and `CoxeterGroup`, and finally\n\n3. make `WeylGroup` and `CoxeterGroup` compute the number of reflections in the fastest possible way.",
"created_at": "2016-07-06T12:01:39Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304313",
"user": "https://github.com/stumpc5"
}comment:9
Okay, I guess you are right. I should and will do it properly:
-
make the method
ReflectionGroup.number_of_reflectionsuse the attributeReflectionGroup._number_of_reflectionsinstead of recomputing it, -
make this flip code use the method instead of the attribute for
ReflectionGroupand also use this attribute forWeylGroupandCoxeterGroup, and finally -
make
WeylGroupandCoxeterGroupcompute the number of reflections in the fastest possible way.
archive/issue_comments_304314.json:
{
"body": "<div id=\"comment:10\"></div>\n\nBranch pushed to git repo; I updated commit sha1. New commits:\n<table><tr><td><a href=\"https://github.com/sagemath/sagetrac-mirror/commit/f17422a1df8f9a0d15f7d51f7340f416625c4417\"><code>f17422a</code></a></td><td><code>working out the proper way to compute the number of reflections</code></td></tr></table>\n",
"created_at": "2016-07-06T12:10:50Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304314",
"user": "https://github.com/sagetrac-git"
}Branch pushed to git repo; I updated commit sha1. New commits:
f17422a | working out the proper way to compute the number of reflections |
archive/issue_comments_304315.json:
{
"body": "Changed commit from **[`aaf771e`](https://github.com/sagemath/sagetrac-mirror/commit/aaf771ed669652a1b09d69b5e7c539afa950120a)** to **[`f17422a`](https://github.com/sagemath/sagetrac-mirror/commit/f17422a1df8f9a0d15f7d51f7340f416625c4417)**",
"created_at": "2016-07-06T12:10:50Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304315",
"user": "https://github.com/sagetrac-git"
}Changed commit from aaf771e to f17422a
archive/issue_comments_304316.json:
{
"body": "Changed commit from **[`f17422a`](https://github.com/sagemath/sagetrac-mirror/commit/f17422a1df8f9a0d15f7d51f7340f416625c4417)** to **[`6e97757`](https://github.com/sagemath/sagetrac-mirror/commit/6e97757e2d1181e51ea381a49161e157b6df5418)**",
"created_at": "2016-07-06T12:14:13Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304316",
"user": "https://github.com/sagetrac-git"
}Changed commit from f17422a to 6e97757
archive/issue_comments_304317.json:
{
"body": "<div id=\"comment:11\"></div>\n\nBranch pushed to git repo; I updated commit sha1. New commits:\n<table><tr><td><a href=\"https://github.com/sagemath/sagetrac-mirror/commit/6e97757e2d1181e51ea381a49161e157b6df5418\"><code>6e97757</code></a></td><td><code>updated the new doctests to be optional</code></td></tr></table>\n",
"created_at": "2016-07-06T12:14:13Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304317",
"user": "https://github.com/sagetrac-git"
}Branch pushed to git repo; I updated commit sha1. New commits:
6e97757 | updated the new doctests to be optional |
archive/issue_comments_304318.json:
{
"body": "<div id=\"comment:12\" align=\"right\">comment:12</div>\n\nok. Looks much better.\n\nHave you re-done the timings to compare the new \"number_of_reflections\"\nwith the previously used len of the longest word ?",
"created_at": "2016-07-06T12:20:13Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304318",
"user": "https://github.com/fchapoton"
}comment:12
ok. Looks much better.
Have you re-done the timings to compare the new "number_of_reflections" with the previously used len of the longest word ?
archive/issue_comments_304319.json:
{
"body": "Changed commit from **[`6e97757`](https://github.com/sagemath/sagetrac-mirror/commit/6e97757e2d1181e51ea381a49161e157b6df5418)** to **[`35a98d7`](https://github.com/sagemath/sagetrac-mirror/commit/35a98d714455b0c2408e0420550997c0f7232dd9)**",
"created_at": "2016-07-06T12:22:05Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304319",
"user": "https://github.com/sagetrac-git"
}Changed commit from 6e97757 to 35a98d7
archive/issue_comments_304320.json:
{
"body": "<div id=\"comment:13\"></div>\n\nBranch pushed to git repo; I updated commit sha1. New commits:\n<table><tr><td><a href=\"https://github.com/sagemath/sagetrac-mirror/commit/35a98d714455b0c2408e0420550997c0f7232dd9\"><code>35a98d7</code></a></td><td><code>fixed the new doctests, all tests seem to pass now</code></td></tr></table>\n",
"created_at": "2016-07-06T12:22:05Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304320",
"user": "https://github.com/sagetrac-git"
}Branch pushed to git repo; I updated commit sha1. New commits:
35a98d7 | fixed the new doctests, all tests seem to pass now |
archive/issue_comments_304321.json:
{
"body": "<div id=\"comment:14\" align=\"right\">comment:14</div>\n\nThe timings should still be the old as the change\n\n```\n- return ZZ.sum(self.degrees()) - self.rank()\n+ return ZZ.sum(deg-1 for deg in self.degrees())\n```\nshould not make any difference... (just checked, no change there). It would be better if we either do a speed improvement of the `degrees` method for `WeylGroup` and `CoxeterGroup`, or make `number_of_reflections` there use the length of the longest element there instead.",
"created_at": "2016-07-06T12:25:44Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304321",
"user": "https://github.com/stumpc5"
}comment:14
The timings should still be the old as the change
- return ZZ.sum(self.degrees()) - self.rank()
+ return ZZ.sum(deg-1 for deg in self.degrees())
should not make any difference... (just checked, no change there). It would be better if we either do a speed improvement of the degrees method for WeylGroup and CoxeterGroup, or make number_of_reflections there use the length of the longest element there instead.
archive/issue_comments_304322.json:
{
"body": "<div id=\"comment:15\" align=\"right\">comment:15</div>\n\nBut I think that should go into a different ticket.",
"created_at": "2016-07-06T12:26:27Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304322",
"user": "https://github.com/stumpc5"
}comment:15
But I think that should go into a different ticket.
archive/issue_comments_304323.json:
{
"body": "<div id=\"comment:16\" align=\"right\">comment:16</div>\n\nReplying to [@stumpc5](#comment%3A15):\n> But I think that should go into a different ticket.\n\nok, indeed.",
"created_at": "2016-07-06T12:29:17Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304323",
"user": "https://github.com/fchapoton"
}comment:16
Replying to @stumpc5:
But I think that should go into a different ticket.
ok, indeed.
archive/issue_comments_304324.json:
{
"body": "<div id=\"comment:17\" align=\"right\">comment:17</div>\n\nAn idea: why not make `number_of_reflection` a `cached_method` in all cases?",
"created_at": "2016-07-06T12:31:41Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304324",
"user": "https://github.com/fchapoton"
}comment:17
An idea: why not make number_of_reflection a cached_method in all cases?
archive/issue_comments_304325.json:
{
"body": "<div id=\"comment:18\" align=\"right\">comment:18</div>\n\nThat is now #20956.",
"created_at": "2016-07-06T12:34:10Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304325",
"user": "https://github.com/stumpc5"
}comment:18
That is now #20956.
archive/issue_comments_304326.json:
{
"body": "<div id=\"comment:19\" align=\"right\">comment:19</div>\n\nReplying to [@fchapoton](#comment%3A17):\n> An idea: why not make `number_of_reflection` a `cached_method` in all cases?\n\nWhy not, we could also do that. Maybe that's even the \"right\" solution, as the caching doesn't take any memory while avoids recomputing a bigger computation again and again.",
"created_at": "2016-07-06T12:36:08Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304326",
"user": "https://github.com/stumpc5"
}comment:19
Replying to @fchapoton:
An idea: why not make
number_of_reflectionacached_methodin all cases?
Why not, we could also do that. Maybe that's even the "right" solution, as the caching doesn't take any memory while avoids recomputing a bigger computation again and again.
archive/issue_comments_304327.json:
{
"body": "<div id=\"comment:20\" align=\"right\">comment:20</div>\n\nIf you do not object, I would add this here to this ticket and make the other a duplicate.",
"created_at": "2016-07-06T12:36:35Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304327",
"user": "https://github.com/stumpc5"
}comment:20
If you do not object, I would add this here to this ticket and make the other a duplicate.
archive/issue_comments_304328.json:
{
"body": "<div id=\"comment:21\"></div>\n\nBranch pushed to git repo; I updated commit sha1. New commits:\n<table><tr><td><a href=\"https://github.com/sagemath/sagetrac-mirror/commit/250aaee516c3ee3cee3751090e5435d94b79b998\"><code>250aaee</code></a></td><td><code>added cached methods to number of reflections/hyperplanes and rank</code></td></tr></table>\n",
"created_at": "2016-07-06T12:43:14Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304328",
"user": "https://github.com/sagetrac-git"
}Branch pushed to git repo; I updated commit sha1. New commits:
250aaee | added cached methods to number of reflections/hyperplanes and rank |
archive/issue_comments_304329.json:
{
"body": "Changed commit from **[`35a98d7`](https://github.com/sagemath/sagetrac-mirror/commit/35a98d714455b0c2408e0420550997c0f7232dd9)** to **[`250aaee`](https://github.com/sagemath/sagetrac-mirror/commit/250aaee516c3ee3cee3751090e5435d94b79b998)**",
"created_at": "2016-07-06T12:43:14Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304329",
"user": "https://github.com/sagetrac-git"
}Changed commit from 35a98d7 to 250aaee
archive/issue_comments_304330.json:
{
"body": "<div id=\"comment:22\" align=\"right\">comment:22</div>\n\nok, then you can get rid of the attribute and the method in reflection_group_complex",
"created_at": "2016-07-06T12:45:19Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304330",
"user": "https://github.com/fchapoton"
}comment:22
ok, then you can get rid of the attribute and the method in reflection_group_complex
archive/issue_comments_304331.json:
{
"body": "<div id=\"comment:23\" align=\"right\">comment:23</div>\n\nReplying to [@fchapoton](#comment%3A22):\n> ok, then you can get rid of the attribute and the method in reflection_group_complex\n\nOut of curiosity: is it faster to use a cached method, or to use an attribute? Or is that difference never important?",
"created_at": "2016-07-06T12:57:54Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304331",
"user": "https://github.com/stumpc5"
}comment:23
Replying to @fchapoton:
ok, then you can get rid of the attribute and the method in reflection_group_complex
Out of curiosity: is it faster to use a cached method, or to use an attribute? Or is that difference never important?
archive/issue_comments_304332.json:
{
"body": "<div id=\"comment:24\" align=\"right\">comment:24</div>\n\nI do not know, sorry. I guess both are fast enough for our purposes, but we can aim\nfor simplicity for the moment, maybe ?",
"created_at": "2016-07-06T13:01:30Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304332",
"user": "https://github.com/fchapoton"
}comment:24
I do not know, sorry. I guess both are fast enough for our purposes, but we can aim for simplicity for the moment, maybe ?
archive/issue_comments_304333.json:
{
"body": "Changed commit from **[`250aaee`](https://github.com/sagemath/sagetrac-mirror/commit/250aaee516c3ee3cee3751090e5435d94b79b998)** to **[`6f2172b`](https://github.com/sagemath/sagetrac-mirror/commit/6f2172b794ec53d55bd7920125b92662dc17edc8)**",
"created_at": "2016-07-06T13:04:28Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304333",
"user": "https://github.com/sagetrac-git"
}Changed commit from 250aaee to 6f2172b
archive/issue_comments_304334.json:
{
"body": "<div id=\"comment:25\"></div>\n\nBranch pushed to git repo; I updated commit sha1. New commits:\n<table><tr><td><a href=\"https://github.com/sagemath/sagetrac-mirror/commit/6f2172b794ec53d55bd7920125b92662dc17edc8\"><code>6f2172b</code></a></td><td><code>replaced _number_of_reflections by number_of_reflections() everywhere</code></td></tr></table>\n",
"created_at": "2016-07-06T13:04:28Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304334",
"user": "https://github.com/sagetrac-git"
}Branch pushed to git repo; I updated commit sha1. New commits:
6f2172b | replaced _number_of_reflections by number_of_reflections() everywhere |
archive/issue_comments_304335.json:
{
"body": "<div id=\"comment:26\" align=\"right\">comment:26</div>\n\nok, let it be.",
"created_at": "2016-07-06T13:20:48Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304335",
"user": "https://github.com/fchapoton"
}comment:26
ok, let it be.
archive/issue_comments_304336.json:
{
"body": "Reviewer: **Fr\u00e9d\u00e9ric Chapoton**",
"created_at": "2016-07-06T13:20:48Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304336",
"user": "https://github.com/fchapoton"
}Reviewer: Frédéric Chapoton
archive/issue_events_292103.json:
{
"actor": "https://github.com/fchapoton",
"created_at": "2016-07-06T13:20:48Z",
"event": "unlabeled",
"issue": "https://github.com/sagemath/sage/issues/20943",
"label": "https://github.com/sagemath/sage/labels/needs%20review",
"label_color": "7fff00",
"label_name": "needs review",
"label_text_color": "ffffff",
"type": "issue_event",
"url": "https://github.com/sagemath/sage/issues/20943#event-292103"
}archive/issue_events_292104.json:
{
"actor": "https://github.com/fchapoton",
"created_at": "2016-07-06T13:20:48Z",
"event": "labeled",
"issue": "https://github.com/sagemath/sage/issues/20943",
"label": "https://github.com/sagemath/sage/labels/positive%20review",
"label_color": "dfffc0",
"label_name": "positive review",
"label_text_color": "ffffff",
"type": "issue_event",
"url": "https://github.com/sagemath/sage/issues/20943#event-292104"
}archive/issue_comments_304337.json:
{
"body": "<div id=\"comment:27\" align=\"right\">comment:27</div>\n\nReplying to [@fchapoton](#comment%3A26):\n> ok, let it be.\n\nthanks!",
"created_at": "2016-07-06T13:21:50Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304337",
"user": "https://github.com/stumpc5"
}comment:27
Replying to @fchapoton:
ok, let it be.
thanks!
archive/issue_comments_304338.json:
{
"body": "<div id=\"comment:28\" align=\"right\">comment:28</div>\n\nJust FYI - calling a `@cached_method` is ~10% slower than a (Python) attribute lookup:\n\n```python\nsage: class Foo(object):\n....: def __init__(self, x):\n....: self.x = x\n....: self._y = x\n....: @cached_method\n....: def y(self):\n....: return self._y\n....: \nsage: F = Foo(5)\nsage: %timeit F.x\nThe slowest run took 73.41 times longer than the fastest. This could mean that an intermediate result is being cached.\n10000000 loops, best of 3: 42.2 ns per loop\nsage: %timeit F.y()\nThe slowest run took 2799.35 times longer than the fastest. This could mean that an intermediate result is being cached.\n10000000 loops, best of 3: 57.1 ns per loop\n```\nHowever, it is better code (IMO) to use the cached method mechanism, and it makes it easier to change/override it.",
"created_at": "2016-07-06T21:09:34Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304338",
"user": "https://github.com/tscrim"
}comment:28
Just FYI - calling a @cached_method is ~10% slower than a (Python) attribute lookup:
sage: class Foo(object):
....: def __init__(self, x):
....: self.x = x
....: self._y = x
....: @cached_method
....: def y(self):
....: return self._y
....:
sage: F = Foo(5)
sage: %timeit F.x
The slowest run took 73.41 times longer than the fastest. This could mean that an intermediate result is being cached.
10000000 loops, best of 3: 42.2 ns per loop
sage: %timeit F.y()
The slowest run took 2799.35 times longer than the fastest. This could mean that an intermediate result is being cached.
10000000 loops, best of 3: 57.1 ns per loopHowever, it is better code (IMO) to use the cached method mechanism, and it makes it easier to change/override it.
archive/issue_events_292105.json:
{
"actor": "https://github.com/vbraun",
"created_at": "2016-07-08T07:09:49Z",
"event": "unlabeled",
"issue": "https://github.com/sagemath/sage/issues/20943",
"label": "https://github.com/sagemath/sage/labels/positive%20review",
"label_color": "dfffc0",
"label_name": "positive review",
"label_text_color": "ffffff",
"type": "issue_event",
"url": "https://github.com/sagemath/sage/issues/20943#event-292105"
}archive/issue_events_292106.json:
{
"actor": "https://github.com/vbraun",
"commit_id": "f658e72f900bf85b1b03628680147dfb0ee99d7e",
"commit_repository": "https://github.com/sagemath/sage",
"created_at": "2016-07-08T07:09:49Z",
"event": "closed",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_event",
"url": "https://github.com/sagemath/sage/issues/20943#event-292106"
}archive/issue_comments_304339.json:
{
"body": "Changed branch from **[u/stumpc5/20943](https://github.com/sagemath/sagetrac-mirror/tree/u/stumpc5/20943)** to **[`6f2172b`](https://github.com/sagemath/sagetrac-mirror/commit/6f2172b794ec53d55bd7920125b92662dc17edc8)**",
"created_at": "2016-07-08T07:09:49Z",
"formatter": "markdown",
"issue": "https://github.com/sagemath/sage/issues/20943",
"type": "issue_comment",
"url": "https://github.com/sagemath/sage/issues/20943#issuecomment-304339",
"user": "https://github.com/vbraun"
}Changed branch from u/stumpc5/20943 to 6f2172b