Opened 11 years ago

Closed 6 years ago

Last modified 5 years ago

#12473 closed defect (fixed)

Remove nested functions in ratpoints

Reported by: ohanar Owned by: GeorgSWeber
Priority: major Milestone: sage-7.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:

Status badges

Description (last modified by jdemeyer)

these won't work for most other compilers besides GCC.

Change History (30)

comment:1 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:2 Changed 9 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:3 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:4 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:5 Changed 6 years ago by dimpase

  • Cc fbissey added
  • Milestone changed from sage-6.4 to sage-7.4

here is a fix : http://users.ox.ac.uk/~coml0531/sage/ratpoints-2.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.

comment:6 Changed 6 years ago by fbissey

Seems to works. I'll work on turning into a patch later.

comment:7 Changed 6 years ago by dimpase

  • Authors set to Dima Pasechnik
  • 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:

4889e9cun-nested and added const qualifiers to as many params as possible

comment:8 Changed 6 years ago by fbissey

  • Branch changed from u/dimpase/ratpts_nonestedfun to u/fbissey/ratpts_nonestedfun
  • Commit changed from 4889e9c61580b1e894e67505ca2bfb4da9024712 to 32c21d9595c7d180fbea0f86b05045967a750939

Just touching spkg-install since -fnested-functions is not needed anywhere anymore.


New commits:

32c21d9Removing fnested-functions from cflags no that the nested functions are removed.

comment:9 Changed 6 years ago by fbissey

  • Status changed from needs_review to positive_review

comment:10 Changed 6 years ago by dimpase

  • Reviewers set to François Bissey

comment:11 Changed 6 years ago by dimpase

Thanks!

comment:12 Changed 6 years ago by fbissey

I meant to do that ;)

comment:13 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

You should bump the package version number if you make non-trivial changes.

comment:14 Changed 6 years ago by jdemeyer

  • Component changed from build to packages: standard

comment:15 follow-up: Changed 6 years ago by jdemeyer

  • 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 dimpase

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 follow-up: Changed 6 years ago by fbissey

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 git

  • Commit changed from 32c21d9595c7d180fbea0f86b05045967a750939 to dae689cd2b3f68a703fc8c00ae8bb942d653e464

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

4889e9cun-nested and added const qualifiers to as many params as possible
15e20ffMerge branch 'u/dimpase/ratpts_nonestedfun' of trac.sagemath.org:sage into ratpts_nonestedfun
dae689cVersion bump to force rebuild

comment:19 Changed 6 years ago by fbissey

  • Status changed from needs_work to needs_review

Should all be better now.

comment:20 Changed 6 years ago by dimpase

  • Status changed from needs_review to positive_review

OK, looks good.

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

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 vbraun

  • 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 dimpase

  • Commit dae689cd2b3f68a703fc8c00ae8bb942d653e464 deleted
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:24 Changed 5 years ago by jdemeyer

Just commenting that the functionality that ratpoints provides should be replaced with PARI. It seems that PARI is slowly replacing all number-theoretical packages inside Sage. This already happened with genus2red and it should be possible for lcalc and ratpoints too.

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

replacing sounds a bit drastic; adding another backend?

comment:26 in reply to: ↑ 25 ; follow-up: Changed 5 years ago by 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, can provide the same functionality is not controversial in my opinion. Who doesn't like less maintenance.

comment:27 follow-up: Changed 5 years ago by dimpase

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 jdemeyer

I opened #24531 for that, by the way.

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

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 jdemeyer

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.

Note: See TracTickets for help on using tickets.