Opened 8 years ago

Closed 8 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, GitHub, GitLab) Commit: c46a4d569c3fea2235af809235b5d8eeb6a4b34e
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

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

Change History (42)

comment:1 Changed 8 years ago by jdemeyer

Cc: vbraun mmasdeu was added

comment:2 Changed 8 years ago by SimonKing

Cc: SimonKing added

comment:3 Changed 8 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 8 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 8 years ago by vbraun

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

comment:6 Changed 8 years ago by vbraun

Branch: u/vbraun/add_missing_fmpz_init

comment:7 Changed 8 years ago by vbraun

Commit: 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 8 years ago by jdemeyer

Description: modified (diff)

comment:9 Changed 8 years ago by jdemeyer

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

comment:10 in reply to:  9 Changed 8 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 8 years ago by SimonKing

Why not building a static library, by the way?

comment:12 Changed 8 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 Changed 8 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 8 years ago by jdemeyer

Branch: u/vbraun/add_missing_fmpz_initu/jdemeyer/ticket/17090
Created: Oct 2, 2014, 1:59:57 PMOct 2, 2014, 1:59:57 PM
Modified: Oct 2, 2014, 3:28:42 PMOct 2, 2014, 3:28:42 PM

comment:15 Changed 8 years ago by jdemeyer

Commit: df1045910e8324a19b7efb748d884327be4567cc7b38491f4e99b814b6c238d6c96d7cb263b77e77

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


New commits:

7b38491Fix initialization of fmpz_t

comment:16 Changed 8 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 Changed 8 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 ; Changed 8 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 8 years ago by jdemeyer

Description: modified (diff)
Summary: Add missing fmpz_init() statements after #16803Fix issues with #16803

comment:20 Changed 8 years ago by vbraun

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

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

comment:21 Changed 8 years ago by git

Commit: 7b38491f4e99b814b6c238d6c96d7cb263b77e778a16a8ea3e5abf5ea6d2fbe04bbdd670abb42619

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

8a16a8eFurther clean-up

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

Status: newneeds_review

Replying to SimonKing:

I think it should be used.

Done

comment:23 Changed 8 years ago by git

Commit: 8a16a8ea3e5abf5ea6d2fbe04bbdd670abb42619e5f684b2345e457791a1d878178cb5061764b421

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

e5f684bRemove unused code

comment:24 Changed 8 years ago by git

Commit: e5f684b2345e457791a1d878178cb5061764b4219b310031e9d55973b13aa5876756589454a05593

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

9b31003Check 'implementation' argument

comment:25 Changed 8 years ago by git

Commit: 9b310031e9d55973b13aa5876756589454a05593a2860d835bf647e7f84b876302c06c88eab67c51

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

a2860d8Undo changes to padics from #16803

comment:26 Changed 8 years ago by jdemeyer

Authors: Volker Braun, Jeroen Demeyer

comment:27 Changed 8 years ago by git

Commit: a2860d835bf647e7f84b876302c06c88eab67c51342d1520fbdb36041b1dc7e5a65c62e91dfb1e85

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

342d152Remove unneeded imports

comment:28 Changed 8 years ago by jdemeyer

Summary: Fix issues with #16803Reviewer patch for #16803

comment:29 Changed 8 years ago by was

Why did you delete the following functions:

_multiply_linbox
_multiply_multi_modular
_det_pari

comment:30 in reply to:  29 ; Changed 8 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 8 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 8 years ago by git

Commit: 342d1520fbdb36041b1dc7e5a65c62e91dfb1e855b77c7816073fd46c915a05eb6e1461ea02fcd3a

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 8 years ago by git

Commit: 5b77c7816073fd46c915a05eb6e1461ea02fcd3a7843e3f1d2021ecf82e3384f0217fce4c5edeec3

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

7843e3fFix fmpz_mat_t declaration

comment:34 Changed 8 years ago by git

Commit: 7843e3f1d2021ecf82e3384f0217fce4c5edeec3c46a4d569c3fea2235af809235b5d8eeb6a4b34e

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 8 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 8 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 ; Changed 8 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 ; Changed 8 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 8 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 8 years ago by vbraun

Status: needs_reviewpositive_review

Looks good to me!

comment:41 Changed 8 years ago by vbraun

Reviewers: Volker Braun, Jeroen Demeyer

comment:42 Changed 8 years ago by vbraun

Branch: u/jdemeyer/ticket/17090c46a4d569c3fea2235af809235b5d8eeb6a4b34e
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.