Opened 7 years ago
Closed 7 years ago
#13184 closed defect (fixed)
Some Homset are not unique parents
Reported by: | caruso | Owned by: | nthiery |
---|---|---|---|
Priority: | major | Milestone: | sage-5.10 |
Component: | categories | Keywords: | homset unique parent |
Cc: | caruso, SimonKing, nbruin | Merged in: | sage-5.10.beta0 |
Authors: | Xavier Caruso, Frédéric Chapoton | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #14159 | Stopgaps: |
Description (last modified by )
I guess it is a bug:
sage: k = GF(5) sage: H = Hom(k,k) sage: H2 = Hom(k,k) sage: H is H2 False
I don't know what is the correct way to fix this problem.
More precisely, in sage.categories.homset (l. 223-227), one can read:
try: # Apparently X._Hom_ is supposed to be cached return X._Hom_(Y, category) except (AttributeError, TypeError): pass
However, in this particular case, k._Hom_ is apparently not cached. IMHO, caching should be the job of sage.categories.homset.Hom is all cases, but I might be wrong.
Apply
Attachments (4)
Change History (28)
Changed 7 years ago by
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
- Status changed from new to needs_review
comment:3 Changed 7 years ago by
- Cc caruso added
comment:4 Changed 7 years ago by
Please fill in your real name as Author.
comment:5 Changed 7 years ago by
comment:6 Changed 7 years ago by
- Description modified (diff)
comment:7 Changed 7 years ago by
- Status changed from needs_review to needs_work
Sage does not run with the patch applied:
/home/travis/sage-5.5.rc0/local/lib/python2.7/site-packages/sage/categories/homset.pyc in Hom(X, Y, category) 239 # To be investigated 240 H = X._Hom_(Y,category) --> 241 _cache[key] = weakref.ref(H) 242 return H 243 except (AttributeError, TypeError): NameError: global name 'weakref' is not defined Error importing ipy_profile_sage - perhaps you should run %upgrade? WARNING: Loading of ipy_profile_sage failed.
comment:8 Changed 7 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
I don't understand exactly why you got this error.
Nevertheless, I noticed that ticket #11521 was merged in sage-5.5.beta0 resulting in some important modifications in sage/categories/homset.py
. As a consequence, my patch does not apply in versions >= 5.5.beta0. So, I attach a new patch to apply to these recent versions.
Changed 7 years ago by
comment:9 Changed 7 years ago by
- Description modified (diff)
comment:10 Changed 7 years ago by
- Status changed from needs_review to needs_work
- Work issues set to sage fails to run
Same error. I'm running 5.5.rc0
. The patchbot agreed with me on the previous version as well (all doctests basically fail), and I suspect it will agree with me on the new patch. Do you have any patches applied in your queue, or if are running something before 5.5.rc0
, do you have every patch which modifies homset.py
applied before the patch in the queue?
Thanks,
Travis
PS - it's generally not a good idea to have periods in your filenames since they are used for extension types; typically people replace them with underscores.
For patchbot:
Apply only: trac_13184_sage_5.5.0.beta.patch
comment:11 Changed 7 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
here is a rebased and slightly modified patch
for the bot:
apply trac_13184_sage_5.9.beta.patch
comment:12 Changed 7 years ago by
- Status changed from needs_review to needs_work
- Work issues changed from sage fails to run to one doctest
ok, just one little failing doctest, needs to be investigated
comment:13 Changed 7 years ago by
ok, this new patch should pass all tests. Let us wait and see
comment:14 Changed 7 years ago by
for the bot:
apply trac_13184_sage_5.9.beta.patch
comment:15 Changed 7 years ago by
ok, the bot is green. There now remains the question whether or not this is the right thing to do.
Any opinion, somebody ?
comment:16 Changed 7 years ago by
- Cc SimonKing added
- Reviewers set to Travis Scrimshaw
- Status changed from needs_work to needs_review
I'm cc-ing Simon to see if he can find a memory leak that I couldn't. However it looks good to me.
comment:17 Changed 7 years ago by
comment:18 follow-up: ↓ 19 Changed 7 years ago by
- Cc nbruin added
I believe so, however because of the file rename done in #14214, there will be a dependency one way or the other. I'm cc-ing Nils to let him know about this ticket. Enjoy your holiday Simon.
Nils, what is the review status of #14214 and it's dependency #14159, and perhaps you can find a memory leak that I didn't?
Thanks,
Travis
comment:19 in reply to: ↑ 18 Changed 7 years ago by
comment:20 Changed 7 years ago by
- Dependencies set to #14159
Changed 7 years ago by
comment:21 Changed 7 years ago by
here is a patch applying above #14159, where I have made the required change
for the bot : apply trac_13184_sage_5.9.beta.patch
comment:22 Changed 7 years ago by
- Status changed from needs_review to positive_review
- Work issues one doctest deleted
Thank you.
comment:23 Changed 7 years ago by
- Milestone changed from sage-5.9 to sage-5.10
comment:24 Changed 7 years ago by
- Merged in set to sage-5.10.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
I've attached a small patch fixing the problem.