-
-
Notifications
You must be signed in to change notification settings - Fork 481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Categories for all rings #9138
Comments
This comment has been minimized.
This comment has been minimized.
comment:2
This ticket seems to be a duplicate of #8613. |
comment:3
Replying to @mezzarobba:
Indeed. This should have ringed a bell to me! Since I have already recycled this ticket to "Categories for |
comment:4
This ticket is just about a single kind of parent classes. Rather than going through a long list of parent classes one by one and inserting the missing pieces: Wouldn't it be a more thorough approach to provide a default implementation for the attributes needed in the category framework, in cases where it makes sense? Here is an example:
If I am not mistaken, "element_class" should be implemented by providing the attribute "Element". But is there a reason why element_class is a dynamic meta-class and not a regular method? Since any parent class has a "an_element" method, it seems to me that the following default implementation makes sense (and it solves the problem in my example above):
It seems to me that providing reasonable default implementations would, on the long run, be easier than going through any single parent class. But certainly other people know more about the "how-to" of categories. |
comment:5
Replying to @simon-king-jena:
Sorry, I just noticed that "element_class" is not a method at all: I assumed that it should be used like Anyway, if there shall be a default implementation for element_class then unfortunately it must be in |
comment:6
Replying to @simon-king-jena:
Again wrong. I found in sage.structure.parent that there is indeed a method element_class -- with a lazy_attribute decorator. I am still confused by that programming style, so, better I shut up. Anyway, changing the element_class method so that an_element is used (rather than raising an |
comment:7
Question: Is it OK to broaden the scope of this ticket, namely to use the category framework for everything that comes from sage/rings/ring.pyx? Or shall it be restricted to polynomial rings. |
Author: Simon King |
comment:8
Since #9944 almost has a positive review but does not entirely fix the bug reported here, I'm posting my patch here, building on top of that. Examples: With the patches from #9944:
Adding my (not yet submitted) patch, I get:
So, I think the bug reported here is fixed. For me, all doctests pass. Depends on #9944 |
comment:9
Replying to @simon-king-jena:
Oops, I meant: That's with the patch that I have submitted... |
comment:10
Hi Simon, I haven't been following all the details of this, but thanks for the patch! One question I have is whether there is a performance penalty for this, and if so, to what degree is that acceptable. On my machine, I noticed about a 10% slow-down for
after applying the patches at #9944 and here. I did not do any rigorous testing so this may be spurious, but I'm not sure this should be given a positive review unless this issue has at least been considered. If this has already happened and I missed the discussion, then I apologize. Just point me to it and I'll shut up and go away. :) |
comment:11
Hi Jason, Replying to @jbandlow:
You are right, things like this should be considered. However, I wonder where a slow-down might come from.
No, I wasn't testing the performance. Aparently I work in cycles: Recently I had several patches that considerably increased performance, and perhaps I am now in a slow mood again... Anyway, I'll do some tests now. |
comment:12
Here are the test results: Without all patches:
With the patches from #9944:
With the patches from #9944 and the patch from here:
Note, however, that the numbers arent't very stable
But there is a tendency: Things tend to be slower, both with #9944 and with my patch. So, it should be worth while to analyse the arithmetic with prun. |
comment:13
By the way, I think we should add doctests of the type
If I understand the ticket description, this used to fail. |
comment:14
Idea: Could it be that the length of the method resolution order is responsible for the slow-down? With all patches:
With only the patches from #9944:
Without these patches:
So, the mro of the rings becomes much longer. Could it be that, as a consequence, it takes longer to find common and frequently used methods such as |
comment:15
Here are times for basic methods (i.e., methods that require to walk up much of the mro): Without patches
With all patches
So, there seems to be some slow-down in accessing basic methods. |
comment:16
But apparently one can work around the mro:
So, if speed matters, it might be worth while to use the idiom above to speed things up. I'll ask on sage-devel whether there is a more elegant/pythonic way to cope with a long mro. |
comment:17
I had to rebase my patch since it Depends on #9944 |
comment:18
With the latest patches, I obtain the following timings:
Without these patches:
In other words, there is a mild deceleration in the univariate case and a mild (and in one case considerable) acceleration in the multivariate case. I don't understand why. But perhaps a reviewer has an idea, and can also state his or her opinion how bad the deceleration is compared with the acceleration? |
comment:123
A note to the release manager: #11900 just got a positive review by Nicolas Thiéry. |
Attachment: 9138_flat.patch.gz All patches together in one patch, rebased to #9958 |
This comment has been minimized.
This comment has been minimized.
Merged: sage-5.0.beta0 |
Introspection is failing on polynomial rings:
This is because polynomial rings do not yet set their category properly:
See http://groups.google.com/group/sage-devel/browse_thread/thread/4780192a11a8b591 for more discussion.
Many other rings are not properly initialised as well. The aim of this ticket is to change that.
Apply attachment: 9138_flat.patch (rebased on top of #9958, but will still apply with fuzz 2 otherwise).
See #11900 for a follow-up fixing some speed regressions.
Depends on #11900
Dependencies: To be merged with #11900
CC: @sagetrac-sage-combinat @robertwb
Component: categories
Keywords: introspection, categories for rings
Author: Simon King
Reviewer: Volker Braun
Merged: sage-5.0.beta0
Issue created by migration from https://trac.sagemath.org/ticket/9138
The text was updated successfully, but these errors were encountered: