Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up partitions_c.cc #24667

Closed
jdemeyer opened this issue Feb 6, 2018 · 27 comments
Closed

Clean up partitions_c.cc #24667

jdemeyer opened this issue Feb 6, 2018 · 27 comments

Comments

@jdemeyer
Copy link

jdemeyer commented Feb 6, 2018

  1. Remove some unused stuff.

  2. Compute the double and long double constants using MPFR. This makes a lot of sense since we already computed MPFR versions of these constants anyway. For extra safety, we also increase the precision used to compute these constants.

  3. No longer use the header file partitions_c.h. Instead, include the .cc file in Cython.

Points 2. and 3. above allow to remove some platform-specific hacks.

  1. Replace T(int(x)) by T(x) to convert x to type T.

  2. Only one temporary MPFR variable is really needed, call it mptemp.

  3. Merge initialize_mpz_and_mpq_variables, initialize_mpfr_variables and initialize_constants in one function initialize_globals.

  4. Use some more specialized MPFR functions like mpfr_set_prec() where applicable.

  5. Define all internal functions as static.

  6. Use MPFR_RNDF rounding mode where exact rounding is not required.

  7. Ensure that the file compiles without compiler warnings.

  8. Various style improvements.

CC: @dimpase

Component: combinatorics

Author: Jeroen Demeyer

Branch/Commit: 2dba6c8

Reviewer: Dima Pasechnik

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

@jdemeyer jdemeyer added this to the sage-8.2 milestone Feb 6, 2018
@jdemeyer jdemeyer changed the title Remove unused stuff from partitions_c.cc Clean up partitions_c.cc Feb 6, 2018
@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

jdemeyer commented Feb 6, 2018

@jdemeyer
Copy link
Author

jdemeyer commented Feb 6, 2018

New commits:

93de16eClean up partitions_c.cc

@jdemeyer
Copy link
Author

jdemeyer commented Feb 6, 2018

Commit: 93de16e

@jdemeyer

This comment has been minimized.

@dimpase
Copy link
Member

dimpase commented Feb 7, 2018

comment:16

in this chunk

@@ -83,16 +86,6 @@
  *      along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-
-#if defined(__sun) || defined(__CYGWIN__)
-extern "C" long double fabsl (long double);
-extern "C" long double sinl (long double);
-extern "C" long double cosl (long double);
-extern "C" long double sqrtl (long double);
-extern "C" long double coshl (long double);
-extern "C" long double sinhl (long double);
-#endif

you're removing Gygwin stuff. Are you sure it's OK?

@jdemeyer
Copy link
Author

jdemeyer commented Feb 7, 2018

comment:17

Replying to @dimpase:

Are you sure it's OK?

Yes because I removed the calls to those functions. That is what point 2 in the ticket description achieves.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 7, 2018

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

2dba6c8Minor fixes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 7, 2018

Changed commit from 93de16e to 2dba6c8

@mezzarobba
Copy link
Member

comment:19

Perhaps a stupid question, sorry, but is there any reason to use this implementation instead of the one in arb?

@jdemeyer
Copy link
Author

comment:20

Replying to @mezzarobba:

Perhaps a stupid question, sorry, but is there any reason to use this implementation instead of the one in arb?

I don't know. I'm just fixing the existing code.

@tscrim
Copy link
Collaborator

tscrim commented Feb 13, 2018

comment:21

Replying to @jdemeyer:

Replying to @mezzarobba:

Perhaps a stupid question, sorry, but is there any reason to use this implementation instead of the one in arb?

I don't know. I'm just fixing the existing code.

IMO, it is not a bad thing to have redundancy checking with independent implementations.

@dimpase
Copy link
Member

dimpase commented Feb 15, 2018

comment:22

naturally, we can have several backends to computing a function. Patches are welcome.

@dimpase
Copy link
Member

dimpase commented Feb 15, 2018

Reviewer: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Feb 15, 2018

comment:23

looks good to me. Send it to the bots!

@vbraun
Copy link
Member

vbraun commented Feb 16, 2018

Changed branch from u/jdemeyer/remove_unused_stuff_from_partitions_c_cc to 2dba6c8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants