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: |
Description (last modified by )
Various fixes/improvements/clean-ups after #16803.
Change History (42)
comment:1 Changed 8 years ago by
Cc: | vbraun mmasdeu was added |
---|
comment:2 Changed 8 years ago by
Cc: | SimonKing added |
---|
comment:3 follow-up: 4 Changed 8 years ago by
comment:4 Changed 8 years ago by
comment:5 Changed 8 years ago by
I'd prefer to add it here, there is already too much going on at #16938
comment:6 Changed 8 years ago by
Branch: | → u/vbraun/add_missing_fmpz_init |
---|
comment:7 Changed 8 years ago by
Commit: | → df1045910e8324a19b7efb748d884327be4567cc |
---|
comment:8 Changed 8 years ago by
Description: | modified (diff) |
---|
comment:9 follow-up: 10 Changed 8 years ago by
The problem is that an assertion won't help against uninitialized variables.
comment:10 Changed 8 years ago by
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:12 Changed 8 years ago by
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: 37 Changed 8 years ago by
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
Branch: | u/vbraun/add_missing_fmpz_init → u/jdemeyer/ticket/17090 |
---|---|
Created: | Oct 2, 2014, 1:59:57 PM → Oct 2, 2014, 1:59:57 PM |
Modified: | Oct 2, 2014, 3:28:42 PM → Oct 2, 2014, 3:28:42 PM |
comment:15 Changed 8 years ago by
Commit: | df1045910e8324a19b7efb748d884327be4567cc → 7b38491f4e99b814b6c238d6c96d7cb263b77e77 |
---|
comment:16 Changed 8 years ago by
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: 18 Changed 8 years ago by
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 follow-up: 22 Changed 8 years ago by
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
Description: | modified (diff) |
---|---|
Summary: | Add missing fmpz_init() statements after #16803 → Fix issues with #16803 |
comment:20 Changed 8 years ago by
This branch fixes all valgrind warnings from matrix_integer_dense
(when running benchmark_hnf
)
comment:21 Changed 8 years ago by
Commit: | 7b38491f4e99b814b6c238d6c96d7cb263b77e77 → 8a16a8ea3e5abf5ea6d2fbe04bbdd670abb42619 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
8a16a8e | Further clean-up
|
comment:22 Changed 8 years ago by
Status: | new → needs_review |
---|
comment:23 Changed 8 years ago by
Commit: | 8a16a8ea3e5abf5ea6d2fbe04bbdd670abb42619 → e5f684b2345e457791a1d878178cb5061764b421 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
e5f684b | Remove unused code
|
comment:24 Changed 8 years ago by
Commit: | e5f684b2345e457791a1d878178cb5061764b421 → 9b310031e9d55973b13aa5876756589454a05593 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
9b31003 | Check 'implementation' argument
|
comment:25 Changed 8 years ago by
Commit: | 9b310031e9d55973b13aa5876756589454a05593 → a2860d835bf647e7f84b876302c06c88eab67c51 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
a2860d8 | Undo changes to padics from #16803
|
comment:26 Changed 8 years ago by
Authors: | → Volker Braun, Jeroen Demeyer |
---|
comment:27 Changed 8 years ago by
Commit: | a2860d835bf647e7f84b876302c06c88eab67c51 → 342d1520fbdb36041b1dc7e5a65c62e91dfb1e85 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
342d152 | Remove unneeded imports
|
comment:28 Changed 8 years ago by
Summary: | Fix issues with #16803 → Reviewer patch for #16803 |
---|
comment:29 follow-ups: 30 36 Changed 8 years ago by
Why did you delete the following functions:
_multiply_linbox _multiply_multi_modular _det_pari
comment:30 follow-up: 31 Changed 8 years ago by
comment:31 Changed 8 years ago by
Replying to jdemeyer:
Replying to was:
Why did you delete the following functions:
_multiply_linbox _multiply_multi_modularThese 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
Commit: | 342d1520fbdb36041b1dc7e5a65c62e91dfb1e85 → 5b77c7816073fd46c915a05eb6e1461ea02fcd3a |
---|
comment:33 Changed 8 years ago by
Commit: | 5b77c7816073fd46c915a05eb6e1461ea02fcd3a → 7843e3f1d2021ecf82e3384f0217fce4c5edeec3 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
7843e3f | Fix fmpz_mat_t declaration
|
comment:34 Changed 8 years ago by
Commit: | 7843e3f1d2021ecf82e3384f0217fce4c5edeec3 → c46a4d569c3fea2235af809235b5d8eeb6a4b34e |
---|
comment:35 Changed 8 years ago by
I am going to leave this branch alone for now, please review! There is another follow-up at #17094.
comment:36 Changed 8 years ago by
comment:37 follow-up: 38 Changed 8 years ago by
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 follow-up: 39 Changed 8 years ago by
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 Changed 8 years ago by
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:41 Changed 8 years ago by
Reviewers: | → Volker Braun, Jeroen Demeyer |
---|
comment:42 Changed 8 years ago by
Branch: | u/jdemeyer/ticket/17090 → c46a4d569c3fea2235af809235b5d8eeb6a4b34e |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Flint has a
--enable-assert
configure switch to turn on assertions which should be handy for debugging.