Opened 5 years ago

Closed 5 years ago

#18255 closed defect (fixed)

Remove silly LimitedPrecisionConstant class

Reported by: rws Owned by:
Priority: major Milestone: sage-6.7
Component: symbolics Keywords:
Cc: kcrisman Merged in:
Authors: Ralf Stephan Reviewers: Karl-Dieter Crisman, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 3e8d598 (Commits) Commit: 3e8d5981eb7b787139ab2572e0d9c9e4d6286120
Dependencies: Stopgaps:

Description

brun fails horribly:

sage: brun.n()
NotImplementedError: brun is only available up to 41 bits
sage: brun?
NotImplementedError: brun is only available up to 41 bits
sage: brun??
NotImplementedError: brun is only available up to 41 bits

and this leads to sporadic failure of random_expr, falsifying patchbot test results.

Change History (35)

comment:1 Changed 5 years ago by jdemeyer

  • Summary changed from remove silly Brun constant to Remove silly LimitedPrecisionConstant class

comment:2 Changed 5 years ago by rws

  • Branch set to u/rws/remove_silly_limitedprecisionconstant_class

comment:3 Changed 5 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to 522c48b4611434a86b418ad291b4c5d3fcb0e7c6
  • Status changed from new to needs_review

New commits:

522c48b18255: Remove silly LimitedPrecisionConstant class

comment:4 Changed 5 years ago by jdemeyer

  • Priority changed from critical to minor

comment:5 follow-up: Changed 5 years ago by rws

  • Priority changed from minor to major

It makes patchbot runs useless. Certainly not minor.

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

Replying to rws:

It makes patchbot runs useless.

Really, why?

comment:7 Changed 5 years ago by rws

As in the ticket description this leads to sporadic failure of random_expr.

comment:8 Changed 5 years ago by vdelecroix

  • Status changed from needs_review to needs_work

This is not a sporadic random failure

**********************************************************************
File "src/sage/symbolic/constants.py", line 28, in sage.symbolic.constants
Failed example:
    brun
Exception raised:
    Traceback (most recent call last):
    ...
    NameError: name 'brun' is not defined
**********************************************************************
1 item had failures:
   1 of  55 in sage.symbolic.constants
    [241 tests, 1 failure, 1.91 s]
----------------------------------------------------------------------
sage -t --long --warn-long 71.3 src/sage/symbolic/constants.py  # 1 doctest failed
----------------------------------------------------------------------

comment:9 Changed 5 years ago by git

  • Commit changed from 522c48b4611434a86b418ad291b4c5d3fcb0e7c6 to 3e8d5981eb7b787139ab2572e0d9c9e4d6286120

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

3e8d59818255: remove doctest for removed code

comment:10 Changed 5 years ago by rws

  • Status changed from needs_work to needs_review

comment:11 Changed 5 years ago by jdemeyer

Do you really want to remove the Brun constant itself? The problem is not with brun, the problem is with LimitedPrecisionConstant.

comment:12 Changed 5 years ago by rws

So we add brun = whatever in all.py and get bug reports when a new decimal place is discovered?

comment:13 Changed 5 years ago by vdelecroix

The Brun constant looks like something that we have no idea about excepted its existence... Even the decimals given on oeis do not seem to be proven exact. Do you have a reference for the associated computations? If not, I would rather remove it.

comment:14 Changed 5 years ago by jdemeyer

The decimals are certainly not proven correct, as far as I know it's even an open question whether brun < 2.

So I get your point...

comment:15 Changed 5 years ago by kcrisman

  • Cc kcrisman added

comment:16 follow-up: Changed 5 years ago by kcrisman

Having it in Sage is nice with regard to the Pentium bug, though... (that is, demonstrating even wacky theory has applications).

comment:17 in reply to: ↑ 16 ; follow-up: Changed 5 years ago by vdelecroix

Replying to kcrisman:

Having it in Sage is nice with regard to the Pentium bug, though... (that is, demonstrating even wacky theory has applications).

But then it should not be called brun. What about brun_as_conjectured_by_XYZ? If we had any way to compute it (even very slow) I would be happy to keep it.

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

Replying to vdelecroix:

But then it should not be called brun. What about brun_as_conjectured_by_XYZ?

It's not a conjecture, it's an estimate.

comment:19 follow-up: Changed 5 years ago by kcrisman

I wonder how many failures it would make - running TestSuite would be instructive, if scary.

The decimals are certainly not proven correct, as far as I know it's even an open question whether brun < 2.

Interesting! (Do you have a reference? I can only find references to estimates using twin primes < 10 to the something.)

Are there any (other?) constants known to some number of bits only of potential interest? I bet there are, but maybe it's not worth keeping the class around for that.

(Deprecation? Just asking.)

comment:20 in reply to: ↑ 19 Changed 5 years ago by rws

Replying to kcrisman:

(Deprecation? Just asking.)

Wrong question. It should rather read: will I review and set positive if you add deprecation?

comment:21 Changed 5 years ago by kcrisman

No, because I don't feel qualified to address the question of whether this class should exist. I asked about deprecation because this would be a rather unusual case where the class may not have been meaningful enough to keep around even in deprecated format (bugs aren't deprecated).

comment:22 Changed 5 years ago by rws

If I understand correctly you are of the opinion that deprecation is likely not necessary. Well, I agree.

comment:23 Changed 5 years ago by kcrisman

On the other hand, perhaps someone should check first whether this is in other systems (think mission statement). I'm not sure whether this indicates anything about it in Mma or not.

comment:24 Changed 5 years ago by rws

You mean they have a named string that contains "~2"? Let's pretend they have, would we want to copy it?

comment:25 follow-up: Changed 5 years ago by kcrisman

Well, I just meant whatever they did have - perhaps just for reference. But you are probably right, the whole class is probably not (currently) useful. (But it could be later - maybe we could keep the class, not the constant, in as commented? But again, I'm not feeling qualified to give positive review to any of that - I would certainly support e.g. Jeroen if he did, though.)

comment:26 in reply to: ↑ 25 Changed 5 years ago by jdemeyer

Replying to kcrisman:

maybe we could keep the class, not the constant

The class is buggy, which was the original reason that this ticket was created. If you remove the constant and keep the class, you are just hiding that problem.

comment:27 Changed 5 years ago by kcrisman

I meant just to comment it out but keep it there in commented state so it would be easier for someone to fix. But I don't really care that much, whatever @jdemeyer says is fine with me, because of the unusual case. Where was this even added (rhetorical question)?

comment:28 Changed 5 years ago by jdemeyer

  • Reviewers set to Karl-Dieter Crisman, Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:29 Changed 5 years ago by rws

Thanks for the review!

comment:30 follow-up: Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:31 in reply to: ↑ 30 ; follow-up: Changed 5 years ago by rws

Replying to vbraun:

Merge conflict

Where? Develop merges cleanly.

comment:32 in reply to: ↑ 31 Changed 5 years ago by kcrisman

  • Status changed from needs_work to needs_info

Merge conflict

Where? Develop merges cleanly.

Yeah, Volker, the branch is green... can you at least let us know which ticket?

comment:33 Changed 5 years ago by vbraun

  • Status changed from needs_info to positive_review

comment:34 Changed 5 years ago by vbraun

possibly with something that I unmerged, or maybe wrong ticket

comment:35 Changed 5 years ago by vbraun

  • Branch changed from u/rws/remove_silly_limitedprecisionconstant_class to 3e8d5981eb7b787139ab2572e0d9c9e4d6286120
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.