Opened 4 years ago
Closed 4 years ago
#18255 closed defect (fixed)
Remove silly LimitedPrecisionConstant class
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage6.7 
Component:  symbolics  Keywords:  
Cc:  kcrisman  Merged in:  
Authors:  Ralf Stephan  Reviewers:  KarlDieter 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 4 years ago by
 Summary changed from remove silly Brun constant to Remove silly LimitedPrecisionConstant class
comment:2 Changed 4 years ago by
 Branch set to u/rws/remove_silly_limitedprecisionconstant_class
comment:3 Changed 4 years ago by
 Commit set to 522c48b4611434a86b418ad291b4c5d3fcb0e7c6
 Status changed from new to needs_review
comment:4 Changed 4 years ago by
 Priority changed from critical to minor
comment:5 followup: ↓ 6 Changed 4 years ago by
 Priority changed from minor to major
It makes patchbot runs useless. Certainly not minor.
comment:6 in reply to: ↑ 5 Changed 4 years ago by
comment:7 Changed 4 years ago by
As in the ticket description this leads to sporadic failure of random_expr.
comment:8 Changed 4 years ago by
 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 warnlong 71.3 src/sage/symbolic/constants.py # 1 doctest failed 
comment:9 Changed 4 years ago by
 Commit changed from 522c48b4611434a86b418ad291b4c5d3fcb0e7c6 to 3e8d5981eb7b787139ab2572e0d9c9e4d6286120
Branch pushed to git repo; I updated commit sha1. New commits:
3e8d598  18255: remove doctest for removed code

comment:10 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:11 Changed 4 years ago by
Do you really want to remove the Brun constant itself? The problem is not with brun
, the problem is with LimitedPrecisionConstant
.
comment:12 Changed 4 years ago by
So we add brun = whatever
in all.py
and get bug reports when a new decimal place is discovered?
comment:13 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
 Cc kcrisman added
comment:16 followup: ↓ 17 Changed 4 years ago by
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 ; followup: ↓ 18 Changed 4 years ago by
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 4 years ago by
Replying to vdelecroix:
But then it should not be called
brun
. What aboutbrun_as_conjectured_by_XYZ
?
It's not a conjecture, it's an estimate.
comment:19 followup: ↓ 20 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
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 4 years ago by
If I understand correctly you are of the opinion that deprecation is likely not necessary. Well, I agree.
comment:23 Changed 4 years ago by
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 4 years ago by
You mean they have a named string that contains "~2
"? Let's pretend they have, would we want to copy it?
comment:25 followup: ↓ 26 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
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 4 years ago by
 Reviewers set to KarlDieter Crisman, Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:29 Changed 4 years ago by
Thanks for the review!
comment:30 followup: ↓ 31 Changed 4 years ago by
 Status changed from positive_review to needs_work
Merge conflict
comment:31 in reply to: ↑ 30 ; followup: ↓ 32 Changed 4 years ago by
comment:32 in reply to: ↑ 31 Changed 4 years ago by
 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 4 years ago by
 Status changed from needs_info to positive_review
comment:34 Changed 4 years ago by
possibly with something that I unmerged, or maybe wrong ticket
comment:35 Changed 4 years ago by
 Branch changed from u/rws/remove_silly_limitedprecisionconstant_class to 3e8d5981eb7b787139ab2572e0d9c9e4d6286120
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
18255: Remove silly LimitedPrecisionConstant class