Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#5092 closed defect (fixed)

Primes()?? gets hung in len call; also bring coverage to 100% for primes.py

Reported by: was Owned by: was
Priority: major Milestone: sage-4.2
Component: number theory Keywords:
Cc: Merged in: sage-4.2.alpha1
Authors: William Stein Reviewers: Mike Hansen, Karl-Dieter Crisman
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description


Attachments (1)

trac_5092.patch (2.5 KB) - added by mhansen 10 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 11 years ago by was

  • Summary changed from Primes()?? gets hung in len call to [with patch; needs review] Primes()?? gets hung in len call

comment:2 Changed 11 years ago by was

  • Summary changed from [with patch; needs review] Primes()?? gets hung in len call to [with patch; needs review] Primes()?? gets hung in len call; also bring coverage to 100% for primes.py

comment:3 Changed 11 years ago by saliola

  • Summary changed from [with patch; needs review] Primes()?? gets hung in len call; also bring coverage to 100% for primes.py to [with patch; needs work] Primes()?? gets hung in len call; also bring coverage to 100% for primes.py

You don't need the first two lines below in cmp anymore.

        if isinstance(right, Primes_class):
            return 0
        return cmp(type(self), type(right))

Otherwise, it's a positive review.

comment:4 Changed 11 years ago by saliola

  • Summary changed from [with patch; needs work] Primes()?? gets hung in len call; also bring coverage to 100% for primes.py to [with patch; positive review] Primes()?? gets hung in len call; also bring coverage to 100% for primes.py

comment:5 follow-up: Changed 11 years ago by mabshoff

  • Summary changed from [with patch; positive review] Primes()?? gets hung in len call; also bring coverage to 100% for primes.py to [with patch; needs work] Primes()?? gets hung in len call; also bring coverage to 100% for primes.py

This patch causes one doctest failure:

sage -t -long "devel/sage/sage/sets/set.py"                 
**********************************************************************
File "/scratch/mabshoff/sage-3.3.alpha4/devel/sage/sage/sets/set.py", line 278:
    sage: Primes() < Set(QQ)
Expected:
    True
Got:
    False
**********************************************************************

Cheers,

Michael

comment:6 in reply to: ↑ 5 Changed 11 years ago by saliola

Replying to mabshoff:

This patch causes one doctest failure:

This is a weird test. I'm not even sure that it says anything meaningful. In fact, according to the documentation of cmp for Set, it doesn't:

        \note{If $X < Y$ is true this does \emph{not} necessarily mean
        that $X$ is a subset of $Y$.  Also, any two sets can be
        compared, which is a general Python philosophy.}

comment:7 Changed 10 years ago by AlexGhitza

See #5933, which brings the coverage to 100% and was merged in 3.4.2.rc0.

comment:8 Changed 10 years ago by mabshoff

Well, there is still a potential bug fix in here, so can someone take a look and extra a potential fix?

Cheers,

Michael

Changed 10 years ago by mhansen

comment:9 Changed 10 years ago by mhansen

  • Authors set to William Stein
  • Status changed from needs_work to needs_review

I've rebased the patch of #5933.

comment:10 Changed 10 years ago by mhansen

  • Summary changed from [with patch; needs work] Primes()?? gets hung in len call; also bring coverage to 100% for primes.py to [with patch; needs review] Primes()?? gets hung in len call; also bring coverage to 100% for primes.py

err, on top of #5933

comment:11 Changed 10 years ago by kcrisman

  • Reviewers set to Mike Hansen, Karl-Dieter Crisman
  • Status changed from needs_review to positive_review

Positive review. Unless you can provide an easily accessible example of where Primes(False) isn't Primes(True), but perhaps the first place that happens is waaay down the road...

comment:12 Changed 10 years ago by mhansen

  • Merged in set to sage-4.2.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 Changed 10 years ago by mhansen

  • Summary changed from [with patch; needs review] Primes()?? gets hung in len call; also bring coverage to 100% for primes.py to Primes()?? gets hung in len call; also bring coverage to 100% for primes.py
Note: See TracTickets for help on using tickets.