Opened 5 years ago

Closed 5 years ago

#15757 closed enhancement (fixed)

Make sage.misc.superseded independent of sage.rings.integer_ring

Reported by: nthiery Owned by:
Priority: major Milestone: sage-6.2
Component: misc Keywords:
Cc: Merged in:
Authors: Nicolas M. Thiéry Reviewers: Nathann Cohen, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: u/jdemeyer/ticket/15757 (Commits) Commit: 86568171d749d4fbca7e6fb855fc162377f6cfec
Dependencies: Stopgaps:

Description (last modified by nthiery)

This ticket removes the dependency of sage.misc.superseded upon sage.rings.integer_ring.is_IntegerRing which was used for type checking. This allows for using the functions in this module in code that is imported early upon Sage startup, in particular in category code.

By the way, this removes an unneeded test (the ticket number cannot be None).

Change History (22)

comment:1 Changed 5 years ago by nthiery

  • Branch set to u/nthiery/ticket/15757
  • Created changed from 01/29/14 06:48:06 to 01/29/14 06:48:06
  • Modified changed from 01/29/14 06:48:06 to 01/29/14 06:48:06

comment:2 Changed 5 years ago by nthiery

  • Description modified (diff)
  • Status changed from new to needs_review
  • Summary changed from Make sage.misc.superseded independent of sage.rings.integer_ring to Make sage.misc.superseded lazily import sage.rings.integer_ring
  • Type changed from defect to enhancement

comment:3 Changed 5 years ago by nthiery

  • Authors set to Nicolas M. Thiéry
  • Commit set to db4e2807f18e3b0e13c5101f4531483ba64b6f91

New commits:

db4e28015757: make sage.misc.superseded import lazily sage.rings.integer_ring

comment:4 Changed 5 years ago by nthiery

  • Description modified (diff)

comment:5 Changed 5 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

Yo !

Looks good, no problem. All tests in combinat/ pass on my computer, and the patchbot will check the rest. This being said, you wrote your patch on top of 5.13.rc0 and we are on 6.1.rc0 at the moment ! Are you using 'develop' (and not 'master') ? This caused no conflict here.

Nathann

comment:6 Changed 5 years ago by nthiery

  • Modified changed from 01/29/14 08:38:50 to 01/29/14 08:38:50

Thanks Nathann!

This being said, you wrote your patch on top of 5.13.rc0 and we are on 6.1.rc0 at the moment ! Are you using 'develop' (and not 'master') ? This caused no conflict here.

Oops! I was using develop, but had apparently not pulled on that branch in a (very) long while. Thanks for the notice!

Cheers,

Nicolas

comment:7 follow-up: Changed 5 years ago by jdemeyer

Why not remove _check_trac_number(trac_number) altogether? Perhaps convert the number to int() or use __index__(), which will throw an exception if needed.

comment:8 Changed 5 years ago by ncohen

  • Status changed from positive_review to needs_work

Yep. And as it looks like it is only used once, really ...

What is __index__() though ? O_o

Nathann

comment:9 Changed 5 years ago by ncohen

Oh, it is used several times in the file, sorry. But yeah, looks like we do not need a function just to check that something is a positive integer :-P

There will be false positives anyway as there is no ticket number 9999999999. Yet.

Nathann

comment:10 Changed 5 years ago by jdemeyer

__index__() converts to a Python int, but only if the object really is a kind of integer. x.__index__() is a more strict version of int(x). For example::

sage: ZZ(1).__index__()
1
sage: RR(1).__index__()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-2-9d1cfb1a099d> in <module>()
----> 1 RR(Integer(1)).__index__()

/usr/local/src/sage-git/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.Element.__getattr__ (sage/structure/element.c:3874)()

/usr/local/src/sage-git/local/lib/python2.7/site-packages/sage/structure/misc.so in sage.structure.misc.getattr_from_other_class (sage/structure/misc.c:1696)()

AttributeError: 'sage.rings.real_mpfr.RealNumber' object has no attribute '__index__'
sage: "1".__index__()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-4-8e5c2bcfbfe8> in <module>()
----> 1 "1".__index__()

AttributeError: 'str' object has no attribute '__index__'

comment:11 Changed 5 years ago by git

  • Commit changed from db4e2807f18e3b0e13c5101f4531483ba64b6f91 to c57640dab99e12a8fa94acc49cb4ee478704fcfe

Branch pushed to git repo; I updated commit sha1. New commits:

c57640dCompletely got rid of the dependency of sage.misc.superseded upon is_Integer thanks to suggestion of Jeroen

comment:12 Changed 5 years ago by nthiery

  • Description modified (diff)
  • Modified changed from 01/29/14 20:41:30 to 01/29/14 20:41:30
  • Reviewers changed from Nathann Cohen to Nathann Cohen, Jeroen Demeyer
  • Status changed from needs_work to needs_review

comment:13 in reply to: ↑ 7 Changed 5 years ago by nthiery

Replying to jdemeyer:

Why not remove _check_trac_number(trac_number) altogether? Perhaps convert the number to int() or use __index__(), which will throw an exception if needed.

Ah, thanks for the pointer to index! That's exactly what I was looking for. In a first version, I was using int(), but then "10963" was accepted.

I left _check_trac_number though, since someone had taken the time to write a nice error message, and a .index AttributeError? is not super informative in this case.

Oh, big improvement: now the number must be bigger than 1 :-)

Cheers,

Nicolas

comment:14 Changed 5 years ago by nthiery

  • Summary changed from Make sage.misc.superseded lazily import sage.rings.integer_ring to Make sage.misc.superseded independent of sage.rings.integer_ring

comment:15 follow-up: Changed 5 years ago by jdemeyer

Added a small reviewer patch and merged sage-6.1.rc0.

comment:16 Changed 5 years ago by jdemeyer

  • Branch changed from u/nthiery/ticket/15757 to u/jdemeyer/ticket/15757
  • Modified changed from 01/29/14 21:36:19 to 01/29/14 21:36:19

comment:17 in reply to: ↑ 15 Changed 5 years ago by nthiery

  • Commit changed from c57640dab99e12a8fa94acc49cb4ee478704fcfe to 86568171d749d4fbca7e6fb855fc162377f6cfec

Replying to jdemeyer:

Added a small reviewer patch and merged sage-6.1.rc0.

I just checked out your change, and am happy with them. Thanks!

So I guess that's a positive review assuming the patchpot goes green.


New commits:

e91ee5dMerge remote-tracking branch 'origin/develop' into ticket/15757
8656817Don't use bare except; reviewer patch

comment:18 follow-up: Changed 5 years ago by nthiery

We hit a race condition :-) Unlike what the order above suggests, my comment applies to the latest commit.

Hmm, how to you like the term "patchpot" ?

comment:19 in reply to: ↑ 18 Changed 5 years ago by jdemeyer

Replying to nthiery:

We hit a race condition :-)

Is it really a race condition? I think it's just a plain bug. Changing the branch doesn't automatically add a "new commits" message.

comment:20 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:21 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:22 Changed 5 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.