#12473 closed defect (fixed)
Remove nested functions in ratpoints
Reported by:  ohanar  Owned by:  GeorgSWeber 

Priority:  major  Milestone:  sage7.4 
Component:  packages: standard  Keywords:  
Cc:  fbissey  Merged in:  
Authors:  Dima Pasechnik  Reviewers:  François Bissey 
Report Upstream:  Reported upstream. No feedback yet.  Work issues:  
Branch:  dae689c (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
these won't work for most other compilers besides GCC.
Change History (30)
comment:1 Changed 9 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:2 Changed 9 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:3 Changed 8 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:4 Changed 8 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:5 Changed 6 years ago by
 Cc fbissey added
 Milestone changed from sage6.4 to sage7.4
comment:6 Changed 6 years ago by
Seems to works. I'll work on turning into a patch later.
comment:7 Changed 6 years ago by
 Branch set to u/dimpase/ratpts_nonestedfun
 Commit set to 4889e9c61580b1e894e67505ca2bfb4da9024712
 Status changed from new to needs_review
here is a cleaned up patch, with more const qualifiers added.
New commits:
4889e9c  unnested and added const qualifiers to as many params as possible

comment:8 Changed 6 years ago by
 Branch changed from u/dimpase/ratpts_nonestedfun to u/fbissey/ratpts_nonestedfun
 Commit changed from 4889e9c61580b1e894e67505ca2bfb4da9024712 to 32c21d9595c7d180fbea0f86b05045967a750939
Just touching spkginstall
since fnestedfunctions
is not needed anywhere anymore.
New commits:
32c21d9  Removing fnestedfunctions from cflags no that the nested functions are removed.

comment:9 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:10 Changed 6 years ago by
 Reviewers set to François Bissey
comment:11 Changed 6 years ago by
Thanks!
comment:12 Changed 6 years ago by
I meant to do that ;)
comment:13 Changed 6 years ago by
 Status changed from positive_review to needs_work
You should bump the package version number if you make nontrivial changes.
comment:14 Changed 6 years ago by
 Component changed from build to packages: standard
comment:15 followup: ↓ 16 Changed 6 years ago by
 Description modified (diff)
 Summary changed from remove gcc builtin functions in ratpoints to Remove nested functions in ratpoints
Actually, I have the feeling something is missing on this branch, since it only changes CCFLAGS
but nothing else.
comment:16 in reply to: ↑ 15 Changed 6 years ago by
Replying to jdemeyer:
Actually, I have the feeling something is missing on this branch, since it only changes
CCFLAGS
but nothing else.
Francois, the ball is in your court now... (you didn't ask for a review too :))
comment:17 followup: ↓ 21 Changed 6 years ago by
Yes indeed, it looks like I messed up when importing Dima's branch.
As for the version bump. Yes I know we could do that. But really we didn't change any functionality, we just made it possible to compile with other compilers. But OK the patch is not trivial.
comment:18 Changed 6 years ago by
 Commit changed from 32c21d9595c7d180fbea0f86b05045967a750939 to dae689cd2b3f68a703fc8c00ae8bb942d653e464
comment:19 Changed 6 years ago by
 Status changed from needs_work to needs_review
Should all be better now.
comment:20 Changed 6 years ago by
 Status changed from needs_review to positive_review
OK, looks good.
comment:21 in reply to: ↑ 17 Changed 6 years ago by
Replying to fbissey:
As for the version bump. Yes I know we could do that. But really we didn't change any functionality, we just made it possible to compile with other compilers. But OK the patch is not trivial.
The reason I asked for this is mainly to increase testing coverage. If you bump the version, then everybody who upgrades Sage will rebuild the package with the new patch.
comment:22 Changed 6 years ago by
 Branch changed from u/fbissey/ratpts_nonestedfun to dae689cd2b3f68a703fc8c00ae8bb942d653e464
 Resolution set to fixed
 Status changed from positive_review to closed
comment:23 Changed 6 years ago by
 Commit dae689cd2b3f68a703fc8c00ae8bb942d653e464 deleted
 Report Upstream changed from N/A to Reported upstream. No feedback yet.
comment:24 Changed 5 years ago by
Just commenting that the functionality that ratpoints
provides should be replaced with PARI. It seems that PARI is slowly replacing all numbertheoretical packages inside Sage. This already happened with genus2red
and it should be possible for lcalc
and ratpoints
too.
comment:25 followup: ↓ 26 Changed 5 years ago by
replacing sounds a bit drastic; adding another backend?
comment:26 in reply to: ↑ 25 ; followup: ↓ 30 Changed 5 years ago by
Replying to dimpase:
replacing sounds a bit drastic; adding another backend?
Honestly, it doesn't. Reducing your number of dependencies because another one, better maintained, can provide the same functionality is not controversial in my opinion. Who doesn't like less maintenance.
comment:27 followup: ↓ 29 Changed 5 years ago by
ratpoints implements a very tricky mathematics, not something anybody can write. Having more than one implementation is a blessing.
comment:28 Changed 5 years ago by
I opened #24531 for that, by the way.
comment:29 in reply to: ↑ 27 Changed 5 years ago by
Replying to dimpase:
ratpoints implements a very tricky mathematics, not something anybody can write.
The PARI developers are not just "anybody" :) I trust that they know number theory. Besides, the way that I understand things, the code in PARI is actually based on this ratpoints
package. They didn't write it from scratch.
comment:30 in reply to: ↑ 26 Changed 5 years ago by
Replying to fbissey:
Replying to dimpase:
replacing sounds a bit drastic; adding another backend?
Honestly, it doesn't. Reducing your number of dependencies because another one, better maintained
"better" maintained? I don't think that ratpoints
is maintained at all given that the latest release is from 2011.
here is a fix : http://users.ox.ac.uk/~coml0531/sage/ratpoints2.1.3.tar.bz2 (I moved the only nested function outside; it works with gcc, and passes the tests (in sage)). Wonder how it goes with clang...
Still needs a bit of cleaning.