sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.39k stars 473 forks source link

Remove silly LimitedPrecisionConstant class #18255

Closed rwst closed 9 years ago

rwst commented 9 years ago

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.

CC: @kcrisman

Component: symbolics

Author: Ralf Stephan

Branch/Commit: 3e8d598

Reviewer: Karl-Dieter Crisman, Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/18255

rwst commented 9 years ago

Branch: u/rws/remove_silly_limitedprecisionconstant_class

rwst commented 9 years ago

Commit: 522c48b

rwst commented 9 years ago

Author: Ralf Stephan

rwst commented 9 years ago

New commits:

522c48b18255: Remove silly LimitedPrecisionConstant class
rwst commented 9 years ago
comment:5

It makes patchbot runs useless. Certainly not minor.

jdemeyer commented 9 years ago
comment:6

Replying to @rwst:

It makes patchbot runs useless.

Really, why?

rwst commented 9 years ago
comment:7

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

videlec commented 9 years ago
comment:8

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
----------------------------------------------------------------------
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 522c48b to 3e8d598

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

3e8d59818255: remove doctest for removed code
jdemeyer commented 9 years ago
comment:11

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

rwst commented 9 years ago
comment:12

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

videlec commented 9 years ago
comment:13

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.

jdemeyer commented 9 years ago
comment:14

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...

kcrisman commented 9 years ago
comment:16

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

videlec commented 9 years ago
comment:17

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.

jdemeyer commented 9 years ago
comment:18

Replying to @videlec:

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

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

kcrisman commented 9 years ago
comment:19

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

rwst commented 9 years ago
comment:20

Replying to @kcrisman:

(Deprecation? Just asking.)

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

kcrisman commented 9 years ago
comment:21

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

rwst commented 9 years ago
comment:22

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

kcrisman commented 9 years ago
comment:23

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.

rwst commented 9 years ago
comment:24

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

kcrisman commented 9 years ago
comment:25

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

jdemeyer commented 9 years ago
comment:26

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.

kcrisman commented 9 years ago
comment:27

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

jdemeyer commented 9 years ago

Reviewer: Karl-Dieter Crisman, Jeroen Demeyer

rwst commented 9 years ago
comment:29

Thanks for the review!

vbraun commented 9 years ago
comment:30

Merge conflict

rwst commented 9 years ago
comment:31

Replying to @vbraun:

Merge conflict

Where? Develop merges cleanly.

kcrisman commented 9 years ago
comment:32

Merge conflict

Where? Develop merges cleanly.

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

vbraun commented 9 years ago
comment:34

possibly with something that I unmerged, or maybe wrong ticket

vbraun commented 9 years ago

Changed branch from u/rws/remove_silly_limitedprecisionconstant_class to 3e8d598