Opened 5 years ago

Closed 5 years ago

#17090 closed defect (fixed)

Reviewer patch for #16803

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-6.4
Component: linear algebra Keywords:
Cc: vbraun, mmasdeu, was, SimonKing Merged in:
Authors: Volker Braun, Jeroen Demeyer Reviewers: Volker Braun, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: c46a4d5 (Commits) Commit: c46a4d569c3fea2235af809235b5d8eeb6a4b34e
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Various fixes/improvements/clean-ups after #16803.

Change History (42)

comment:1 Changed 5 years ago by jdemeyer

  • Cc vbraun mmasdeu was added

comment:2 Changed 5 years ago by SimonKing

  • Cc SimonKing added

comment:3 follow-up: Changed 5 years ago by vbraun

Flint has a --enable-assert configure switch to turn on assertions which should be handy for debugging.

comment:4 in reply to: ↑ 3 Changed 5 years ago by SimonKing

Replying to vbraun:

Flint has a --enable-assert configure switch to turn on assertions which should be handy for debugging.

... which should become part of #16938, right?

comment:5 Changed 5 years ago by vbraun

I'd prefer to add it here, there is already too much going on at #16938

comment:6 Changed 5 years ago by vbraun

  • Branch set to u/vbraun/add_missing_fmpz_init

comment:7 Changed 5 years ago by vbraun

  • Commit set to df1045910e8324a19b7efb748d884327be4567cc

Turns out assertions aren't used that much in flint, I still can't reproduce the crash.


New commits:

6f2750eNever build static library
df10459Enable flint assertions

comment:8 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:9 follow-up: Changed 5 years ago by jdemeyer

The problem is that an assertion won't help against uninitialized variables.

comment:10 in reply to: ↑ 9 Changed 5 years ago by SimonKing

Replying to jdemeyer:

The problem is that an assertion won't help against uninitialized variables.

And I thought a debug version would have a paranoid memory management, so that the use of uninitialised variables would result in an immediate abort.

comment:11 Changed 5 years ago by SimonKing

Why not building a static library, by the way?

comment:12 Changed 5 years ago by vbraun

Because static libraries suck. If you rebuild flint with debugging, is Sage going to use the new shared library or have the old static library compiled in?

comment:13 follow-up: Changed 5 years ago by vbraun

And yes, I had hoped that flint had some mechanism to ensure that *_init is called, but no such luck.

comment:14 Changed 5 years ago by jdemeyer

  • Branch changed from u/vbraun/add_missing_fmpz_init to u/jdemeyer/ticket/17090
  • Created changed from 10/02/14 13:59:57 to 10/02/14 13:59:57
  • Modified changed from 10/02/14 15:28:42 to 10/02/14 15:28:42

comment:15 Changed 5 years ago by jdemeyer

  • Commit changed from df1045910e8324a19b7efb748d884327be4567cc to 7b38491f4e99b814b6c238d6c96d7cb263b77e77

This should fix the problem, but I haven't tested it wth #16938.


New commits:

7b38491Fix initialization of fmpz_t

comment:16 Changed 5 years ago by SimonKing

Concerning

+ fmpz_init(z)
+ fmpz_set_mpz(z, x.value)

Is there no fmpz_init_set? I know that there is a combined init-set function for mpz, but I don't know about fmpz.

comment:17 follow-up: Changed 5 years ago by vbraun

There is:

$ grep fmpz_init_set flint-2.4.3/fmpz.h 
void fmpz_init_set(fmpz_t f, const fmpz_t g)
void fmpz_init_set_ui(fmpz_t f, ulong g)
void fmpz_init_set_readonly(fmpz_t f, const mpz_t z);

comment:18 in reply to: ↑ 17 ; follow-up: Changed 5 years ago by SimonKing

Replying to vbraun:

There is:

$ grep fmpz_init_set flint-2.4.3/fmpz.h 
void fmpz_init_set(fmpz_t f, const fmpz_t g)
void fmpz_init_set_ui(fmpz_t f, ulong g)
void fmpz_init_set_readonly(fmpz_t f, const mpz_t z);

I think it should be used.

comment:19 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Add missing fmpz_init() statements after #16803 to Fix issues with #16803

comment:20 Changed 5 years ago by vbraun

This branch fixes all valgrind warnings from matrix_integer_dense (when running benchmark_hnf)

Last edited 5 years ago by vbraun (previous) (diff)

comment:21 Changed 5 years ago by git

  • Commit changed from 7b38491f4e99b814b6c238d6c96d7cb263b77e77 to 8a16a8ea3e5abf5ea6d2fbe04bbdd670abb42619

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

8a16a8eFurther clean-up

comment:22 in reply to: ↑ 18 Changed 5 years ago by jdemeyer

  • Status changed from new to needs_review

Replying to SimonKing:

I think it should be used.

Done

comment:23 Changed 5 years ago by git

  • Commit changed from 8a16a8ea3e5abf5ea6d2fbe04bbdd670abb42619 to e5f684b2345e457791a1d878178cb5061764b421

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

e5f684bRemove unused code

comment:24 Changed 5 years ago by git

  • Commit changed from e5f684b2345e457791a1d878178cb5061764b421 to 9b310031e9d55973b13aa5876756589454a05593

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

9b31003Check 'implementation' argument

comment:25 Changed 5 years ago by git

  • Commit changed from 9b310031e9d55973b13aa5876756589454a05593 to a2860d835bf647e7f84b876302c06c88eab67c51

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

a2860d8Undo changes to padics from #16803

comment:26 Changed 5 years ago by jdemeyer

  • Authors set to Volker Braun, Jeroen Demeyer

comment:27 Changed 5 years ago by git

  • Commit changed from a2860d835bf647e7f84b876302c06c88eab67c51 to 342d1520fbdb36041b1dc7e5a65c62e91dfb1e85

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

342d152Remove unneeded imports

comment:28 Changed 5 years ago by jdemeyer

  • Summary changed from Fix issues with #16803 to Reviewer patch for #16803

comment:29 follow-ups: Changed 5 years ago by was

Why did you delete the following functions:

_multiply_linbox
_multiply_multi_modular
_det_pari

comment:30 in reply to: ↑ 29 ; follow-up: Changed 5 years ago by jdemeyer

Replying to was:

Why did you delete the following functions:

_multiply_linbox
_multiply_multi_modular

These were simply unused. If you think we should keep them "for reference", there is always the git log.

And _det_pari was copied in #16803, I just deleted one of the two copies.

comment:31 in reply to: ↑ 30 Changed 5 years ago by was

Replying to jdemeyer:

Replying to was:

Why did you delete the following functions:

_multiply_linbox
_multiply_multi_modular

These were simply unused. If you think we should keep them "for reference", there is always the git log.

I'm against removing them. Just because they aren't explicitly used elsewhere in Sage, doesn't mean they aren't useful to have. For example,

  • I used them in testing the original patch (having multiple ways of computing products in order to compare) that this is following up on.

  • At one time Linbox was the fastest for multiplying matrices (at least in 2007, when Clement Pernet was my postdoc at UW). Now it's terrible, at least as it is built in Sage. Having the function _multiply_linbox makes it easier to test the speed of linbox versus flint (or multimodular) at any point. If you remove it, then suddenly this becomes more difficult. It's entirely plausible that linbox could again become faster than FLINT, and it would be more difficult to know without this function.
  • I personally don't trust FLINT, and having an easy alternative to test (or swap out for) matrix multiplication makes me happier. I remember clearly times in the past when FLINT gave flat out wrong answers when multiplying large polynomials (burried deep in some p-adic cohomology calculations), and -- when tracking this down -- being able to instrument things so we could easily run the whole big computation with either FLINT or PARI was critical.

comment:32 Changed 5 years ago by git

  • Commit changed from 342d1520fbdb36041b1dc7e5a65c62e91dfb1e85 to 5b77c7816073fd46c915a05eb6e1461ea02fcd3a

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

95b431dPut back old multiplication code (as requested by William Stein)
5b77c78Restore doctest

comment:33 Changed 5 years ago by git

  • Commit changed from 5b77c7816073fd46c915a05eb6e1461ea02fcd3a to 7843e3f1d2021ecf82e3384f0217fce4c5edeec3

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

7843e3fFix fmpz_mat_t declaration

comment:34 Changed 5 years ago by git

  • Commit changed from 7843e3f1d2021ecf82e3384f0217fce4c5edeec3 to c46a4d569c3fea2235af809235b5d8eeb6a4b34e

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

192c8e6Missing comma between command-line arguments
9604069Merge commit '192c8e6f5c6e5f0bd0dd02d51b5f103924a408ef' into ticket/17090
c46a4d5Some reformatting

comment:35 Changed 5 years ago by jdemeyer

I am going to leave this branch alone for now, please review! There is another follow-up at #17094.

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

Replying to was:

Why did you delete the following functions:

_multiply_linbox

Here's a better reason: it's broken!

sage: A = identity_matrix(ZZ,3)
sage: A._multiply_linbox(A)
[0 0 0]
[0 0 0]
[0 0 0]

Fixing this at #17094...

comment:37 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by jdemeyer

Replying to vbraun:

And yes, I had hoped that flint had some mechanism to ensure that *_init is called, but no such luck.

In C, this simply cannot be done. A variable on the stack starts out in a totally unpredictable state.

comment:38 in reply to: ↑ 37 ; follow-up: Changed 5 years ago by vbraun

Replying to jdemeyer:

In C, this simply cannot be done. A variable on the stack starts out in a totally unpredictable state.

Thats of course, strictly speaking, true. But I would be happy to take my chances with a padding int being initialized to 0xdeadbeef and having a random, but usually different, value if it was not initialized.

comment:39 in reply to: ↑ 38 Changed 5 years ago by jdemeyer

Replying to vbraun:

But I would be happy to take my chances with a padding int being initialized to 0xdeadbeef

...if you take care to clear out that 0xdeadbeef after use and assume that the compiler doesn't optimize that clearing away, I guess it would work.

comment:40 Changed 5 years ago by vbraun

  • Status changed from needs_review to positive_review

Looks good to me!

comment:41 Changed 5 years ago by vbraun

  • Reviewers set to Volker Braun, Jeroen Demeyer

comment:42 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/17090 to c46a4d569c3fea2235af809235b5d8eeb6a4b34e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.