Unique representation for homsets #14793
Comments
|
comment:1
To discuss: the exact semantic for homsets of non unique parents |
|
comment:2
Observation: If one uses We need to discuss whether we "simply" fix them, or better continue to compare homsets by equality (not identity) of domain and codomain. In other words, we might end up with just providing a default pickling for homsets, which is currently missing. |
|
comment:3
When using Schemes: Modular: Rings: Modules Others: Getting failures in sage/doctest seems amazing. What to do? |
|
comment:4
Just a crazy idea (brain storm): We do not want However, if both domain and codomain are unique parents, it is a good idea to use So, we could check whether both domain and codomain inherit from In that way, we would automatically have fast homset comparison and hash, for those parts of sage that use unique representation. I am running tests with an according patch now... |
|
comment:5
With what I described in my previous post, I get these errors. Modular: and others: Are these less than before? |
|
comment:6
Let's look at the errors more closely: and this actually looks like it is due to the |
|
comment:7
Interesting! This works on the command line, but fails in a doctest. :-/ |
|
comment:8
I think I've hit this one before when working on homsets. Perhaps I'll find my old solution somewhere. |
|
comment:9
No, I did not find the solution. The problem seems to be that the modular symbols space M is pickled the OLD python way, hence, its Solution: Provide a "proper" pickling for modular symbols. |
|
comment:10
If I recall correctly, people do explicitly not want to use a cache (such as: Here is a minimal example, that fails when using a proper The first line actually is important. With the first line, we get this error in the last line: Hooray, we have a reproducible failure. |
|
comment:11
Better: So, this should be analysable. |
|
comment:12
Here is the problem and a potential solution. While unpickling of the modular symbols M, we need to construct a homset with domain and codomain M. At this point, calling M.category() results in an error, since M.base() returns None and M.category() wants to return Modules(M.base()). Hence, in the code of the Hom function from sage.categories.homset, the lines cat_X = X.category()
cat_Y = Y.category()crash. The point is: When Hom is called in the process of unpickling a homset, then the correct category is supplied. Hence, the argument "category" of Hom is not None. And I think if in this situation Hence, I'd do: Do you think this is a feasible idea? |
|
comment:13
Replying to @simon-king-jena:
Is [...]
If you can't easily fix the category determination then, yes. However, what you're proposing is a hack, so solving it properly should really be preferred. (Other people more knowledgeable on category stuff will probably have a more informed opinion than this generic remark) |
|
comment:14
Replying to @nbruin:
I don't know if it is valid, but at least it is easily possible to get None:
I don't think so---unless modular symbols are always defined over the rationals. That's not my field of mathematical expertise.
Anyway. If it is really the case that the base of modular symbols will always be the rational field, then it is fine. Best regards, |
|
comment:15
Not good. The documentation states: I guess the proper solution would be to provide a |
|
comment:16
Perhaps the following idea works and is clean. What does Python currently do when unpickling a modular symbol? Well, it creates a new instance of the underlying class, and fills its I guess it is possible to compute the base ring from the contents of the And that means, we could create an unpickling function doing the following, where dct is the M = cls.__new__(cls)
ParentWithAdditiveAbelianGens.__init__(M,base=dct['_ModularSymbolsSpace__character'].base_ring(), category=Modules(dct['_ModularSymbolsSpace__character'].base_ring()))
M.__dict__ = copy(dct)
return MWell, I will try it later... |
|
comment:17
Replying to @simon-king-jena: An example in the documentation: The base ring can (in principle at least) be pretty much anything, and the way you're proposing to retrieve it seems to work OK (it seems the character is always there) |
|
comment:18
Replying to @nbruin:
I'm afraid it does not work. The problem is: We need the homset to finish initialisation of the parent, and Python decides to try and create the homset rather early in the reconstruction of the parent. Hence, when we need the character or the base ring or any other information, it is simply not available yet. I tried to provide a Probable I should provide the code that I have so far. In about 24 hours, I guess... And a different approach: We should try to understand why works, while crashes. |
|
comment:19
This works: Continuing the example, this makes it fail: Continuing the example, this makes it work again: Hence, I think the following happens.
This suggests a way out: We could implement "proper" unpickling of It might also be a good idea to use def free_module(self):
"""
Return the free module underlying this ambient Hecke module (the
forgetful functor from Hecke modules to modules over the base ring)
EXAMPLE::
sage: ModularForms(59, 2).free_module()
Vector space of dimension 6 over Rational Field
"""
try:
return self.__free_module
except AttributeError:
M = sage.modules.all.FreeModule(self.base_ring(), self.rank())
self.__free_module = M
return Mwhich uses |
|
comment:20
Arrgh. The obvious approach class HeckeAlgebra_base(sage.rings.commutative_algebra.CommutativeAlgebra):
...
def __reduce__(self):
return HeckeAlgebra, (self.__M,)fails, because again for unpickling the Hecke algebra, one needs that M is sufficiently initialised, because otherwise |
|
comment:21
It is a can of worms. The problem will always arise under the following conditions:
In this situation, unpickling X means that we have an uninitialised copy Y of X and want to call Hom(Y,Y). My previous suggestion was: "In Hom(X,Y,cat), if X.category() fails and cat is given, then trust that X will eventually become sufficiently initialised to see that X is in cat". Perhaps a modified suggestion is better: "In Do you have a better suggestion? I really would not like to implement |
|
comment:22
PS: With the idea exposed in the previous post, the following previously failing examples would work: |
|
comment:23
Some thoughts:
|
|
comment:24
I have attached an experimental patch that implements the new suggestion. With it, the only error in sage.modular is: which is some automatic test suite. The situation in sage.schemes is still not good, as we get: If we are lucky, this boils down to an improper initialisation of the category of some schemes. |
|
comment:25
Indeed: Overloading |
|
comment:37
Travis, how difficult has it been to apply the old patch? Certainly there have been a bunch of conflicts. |
|
comment:38
For the record: I am happy with your changes, Travis. To be on the safe side, I am running tests on my laptop now, but after all you are the reviewer... And sorry that I forgot to add tests for some new methods. |
|
comment:39
Replying to @simon-king-jena:
None; there were no conflicts. Although it might conflict with #10963..... gulp Actually, I noticed that I missed an added |
|
comment:40
Replying to @tscrim:
I see your post only now---and 8 hours are past Anyway, I am adding a test for the |
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
|
comment:42
Test added. |
|
comment:43
PS: I verified that the homsets in the test really use this and no other method for pickling. |
|
comment:44
Good news: #10963 merges cleanly. |
|
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
|
|
Branch pushed to git repo; I updated commit sha1. This was a forced push. Recent commits:
|
|
comment:49
I'm an idiot and merged the wrong branches... |
|
comment:50
Replying to @tscrim:
Doesn't this qualify as "changing history"? You bad boy |
|
comment:51
It's not changing history, just taking a different path |
|
comment:52
I think we're good to go here unless there's something else you can see or think of? |
|
comment:53
Replying to @tscrim:
Just to make sure: We have three commits, namely one that corresponds to the original patch, your review changes, and then my commit adding one doctest? For the record, I agree with your review changes. But after all, you are the reviewer, not I. If you think the code is fine and if all tests pass after merging the develop branch, I'd not oppose to let this be positively reviewed. |
|
comment:54
Replying to @simon-king-jena:
Correct.
I think everything is good, so positive review. Thank you for your work on this Simon. Now #14279 and its dependency (after we finish #10963 and the weak coercions). |
|
Changed branch from public/structure/unique_repr_homsets-14793 to |
The unique representation of homsets is taken care of by
Hom. What's missing is:CC: @sagetrac-sage-combinat @simon-king-jena @jpflori
Component: categories
Author: Simon King
Branch/Commit:
e2d2f16Reviewer: Travis Scrimshaw
Issue created by migration from https://trac.sagemath.org/ticket/14793
The text was updated successfully, but these errors were encountered: