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 chapoton)

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

  1. trac_13184_sage_5.9.beta.patch

Attachments (4)

trac_13184_homset_unique_parent (1.6 KB) - added by caruso 7 years ago.
trac_13184.patch (1.6 KB) - added by saraedum 7 years ago.
added .patch to the existing patch so the patchbot picks it up
trac_13184_sage_5.5.0.beta.patch (1.6 KB) - added by caruso 7 years ago.
trac_13184_sage_5.9.beta.patch (4.2 KB) - added by chapoton 7 years ago.

Download all attachments as: .zip

Change History (28)

Changed 7 years ago by caruso

comment:1 Changed 7 years ago by caruso

I've attached a small patch fixing the problem.

comment:2 Changed 7 years ago by caruso

  • Status changed from new to needs_review

comment:3 Changed 7 years ago by caruso

  • Cc caruso added

comment:4 Changed 7 years ago by jdemeyer

Please fill in your real name as Author.

comment:5 Changed 7 years ago by caruso

  • Authors set to Xavier Caruso

Changed 7 years ago by saraedum

added .patch to the existing patch so the patchbot picks it up

comment:6 Changed 7 years ago by saraedum

  • Description modified (diff)

comment:7 Changed 7 years ago by tscrim

  • 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 caruso

  • 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 caruso

comment:9 Changed 7 years ago by caruso

  • Description modified (diff)

comment:10 Changed 7 years ago by tscrim

  • 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 chapoton

  • 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 chapoton

  • 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 chapoton

ok, this new patch should pass all tests. Let us wait and see

comment:14 Changed 7 years ago by chapoton

for the bot:

apply trac_13184_sage_5.9.beta.patch

comment:15 Changed 7 years ago by chapoton

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 tscrim

  • Authors changed from Xavier Caruso to Xavier Caruso, Frédéric Chapoton
  • 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 SimonKing

Is this orthogonal to #14214 and #14279? In any case, I can not review it now, since I am afraid I have to be in holidays.

comment:18 follow-up: Changed 7 years ago by tscrim

  • 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 nbruin

Replying to tscrim:

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?

If Simon's away you should probably slide this ticket under there. #14214 doesn't fix a bug, it's an enhancement. And I think it needs a little work.

comment:20 Changed 7 years ago by tscrim

  • Dependencies set to #14159

I've made this a dependency of #14214. Thanks Nils.

Frederic, could you change

_cache[key] = KeyedRef(H, _cache.eraser, (signed_id(X),signed_id(Y),signed_id(category)))

to

_cache[key] = H

following #14159 (and remove the commented out line following it)? Thanks.

Changed 7 years ago by chapoton

comment:21 Changed 7 years ago by chapoton

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 tscrim

  • Status changed from needs_review to positive_review
  • Work issues one doctest deleted

Thank you.

comment:23 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.9 to sage-5.10

comment:24 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.10.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.