fix bucket_sorter bug affecting min_degree_ordering #86
Conversation
|
need to redesign my patch.. don't know how to do it yet. the value index datatype may be too small to represent twice the range. so at the same time i'ts unclear to me, why the addressed bug does not affect algorithms such |
|
I think the best thing is if you can make the simplest unit test that fails when it should pass according to the intended behaviour of the component. |
|
On Wed, Jul 25, 2018 at 03:37:49AM -0700, Jeremy W. Murphy wrote:
I think the best thing is if you can make the simplest unit test that
fails when it should pass according to the intended behaviour of the
component.
Thank you.
We do have a unit test against the upstream header in treedec (former
tdlib). It hits the issue, and it is flagged as fail [1]. It is pretty
simple, but it might not be the simplest possible. See the comments in
the code. Please modify and use it for any purpose whatsoever.
NB: treedec ships a modified header, which i think fixes the issue.
similar tests [2,3] witness this.
Please let me know, if/how I can help any further.
[1] https://github.com/freetdi/tdlib/blob/develop/tests/bucket_sorter_2u.cpp
[2] https://github.com/freetdi/tdlib/blob/develop/tests/bucket_sorter_1.cpp
[3] https://github.com/freetdi/tdlib/blob/develop/tests/bucket_sorter_2.cpp
|
|
I think it's best if you put the relevant unit test into this PR. Whoever is maintaining Boost.Graph won't want to look around at code elsewhere on the web. Ideally (if I was the maintainer, which I'm not) I would want to see that a) the unit test is correct and that b) it fails on master but c) passes with the change to the algorithm. |
|
On Wed, Jul 25, 2018 at 04:14:59AM -0700, Jeremy W. Murphy wrote:
I think it's best if you put the relevant unit test into this PR.
Whoever is maintaining Boost.Graph won't want to look around at code
elsewhere on the web. Ideally (if I was the maintainer, which I'm not)
I would want to see that a) the unit test is correct and that b) it
fails on master but c) passes with the change to the algorithm.
I understand that everybodys time is limited, and I consider doing what
you suggest, once I get to it.
To determine whether the unit test is correct, somebody will have to
read it in any case. so, in effect I cannot really save somebody elses
time here, other than the time it took me to find the bug (go figure).
I fold in contributions into several other projects, and I know how
little time is saved by trying to convince contributors to do things
differently. some can't write code, some cant use git, some don't
understand what they do at all.
If you find that this bug is more important to you, go ahead, bring it
to the attention to somebody who is willing to spend time on it, or fill
the forms -- as you suggest. I will eventually, but it's not my
priority, and it hasn't been for a while.
I do appreciate the reminder, in any case. I forgot about this one.
|
|
I'm helping out with the PR backlog. Looks like you have a bugfix, incomplete tests, and no new examples will be needed. Please make your PR complete. This is to let you know and help me prioritize PR's. |
felix-salfelder
force-pushed
the
bucket_sorter
branch
from
652c180 to
ed5a017
Compare
|
thanks. I commited the test from treedec mentioned above. another bug in min_degree_ordering seems related to this. min_degree_ordering assert fails on some empty and on some complete graphs. see the test. |
|
This may be tangled then. The PR needs to be clean and not affect other parts of the implementation. I'm going to hold off on messing with this until after the first two batches of merges. |
|
On Fri, Aug 31, 2018 at 12:01:02PM -0700, jrmarsha wrote:
This may be tangled then. The PR needs to be clean and not affect
other parts of the implementation. I'm going to hold off on messing
with this until after the first two batches of merges.
the fix is unchanged and only touches include/boost/pending/bucket_sorter.hpp.
the min_degree_ordering test shows a side effect only.
|
|
There really shouldn't be unintended side effects. I'm not going to promote a PR that knowingly introduces regressions. |
|
On Fri, Aug 31, 2018 at 12:09:16PM -0700, jrmarsha wrote:
There really shouldn't be unintended side effects. I'm not going to
promote a PR that knowingly introduces regressions.
right now, boost::min_degree_ordering crashes on empty input graphs, or
silently fails if NDEBUG is set.
with my fix it not only doesn't crash, but also produces a correct
ordering. this is just extra evidence. i am not aware of any other side
effects, but i didn't run min degree on all the other graphs yet.
it is clearly tricky. i did mention that sb actually will have to read
the bucket sorter test case, and guess whether the stack corruption and
the md crash is a bug or a feature.
|
|
Ah crud. I'm going to keep this out of the first two batches of merges. I need to sink time into PR. |
felix-salfelder
changed the title
|
@felix-salfelder thanks for adding the unit test. Earlier, you wrote:
Is this still an open implementation issue for this patch? I.e., does it need some work before being merged?
I guess the answer in the end was that it does affect it. |
|
On Sat, Oct 13, 2018 at 08:49:58PM -0700, Jeremy W. Murphy wrote:
> [..] need redesign [..]
Is this still an open implementation issue for this patch? I.e., does
it need some work before being merged?
Thanks Jeremy for digging into this.
looking at it now.. trying to remember.
the patch embeds the head array into the next array to save several
conditionals which would be all over the place. i thought there was a
problem with integer arithmetic overflow. i just can't see it, perhaps
it was in some other code.
somehow related, the patch introduces ugly assignments in the stack
class such as
prev[new_head] = bucket_id + (head - next);
which could possibly be reformulated. does not seem essential.
note that prev is pointing to size_type (hard wired), which is
unnecessary large in most applications. bucket_id can be an unsigned
char. but this is not the overflow...
Bucket sort is not an overly complex algorithm in general: we could
potentially make a new, simpler, generic implementation, depending on
how tightly coupled the buckets are with how Boost.Graph is using it.
(I have a little experience with these algorithms and a little spare
time to work on open source projects at the moment.)
not sure. "generic", "simple" and "fast" are harder to achieve
simultaneously. especially, the current interface is quite useful.
i expect that a new implementation will lead to similar choices and
tradeoffs. i'd suggest to change the current code, if you can think of
a better way. YMMV.
please, if you have time, consider testing this more, including the
shorter integer types. some simple randomised testing would have easily
revealed the original bug.
I guess the answer in the end was that it does affect it [graph::bmd].
a corner case, but yes.
|
|
I just ran the unit test and it returned "broken", which was unexpected. Is that what it's returning for you? |
|
Hmmm, maybe just something to do with the way I compiled it manually, because when I run it as part of the Boost.Graph test suite, it passes. Kinda weird, though. |
|
On Sun, Oct 14, 2018 at 03:02:09AM -0700, Jeremy W. Murphy wrote:
Hmmm, maybe just something to do with the way I compiled it manually,
because when I run it as part of the Boost.Graph test suite, it
passes. Kinda weird, though.
I only have this. never tried to use the test suite.
test$ g++ bucket_sorter_2u.cpp # uses system headers (old)
test$ ./a.out | grep -e good -e broken
broken
test$ g++ bucket_sorter_2u.cpp -I../include
test$ ./a.out | grep -e good -e broken
good
I ticked the "Allow edits from maintainers" box. please feel free.
|
|
OK, yeah, I must have picked up the system headers by accident when I compiled it manually. |
I'm not a maintainer. :) |
|
@felix-salfelder Could you rebase your PR? More CI tests are in devel, as well as other fixes. |
felix-salfelder
force-pushed
the
bucket_sorter
branch
from
c67186c to
7b8de14
Compare
|
This PR is against master, and should be against develop. |
|
@felix-salfelder What is your status on this? |
|
On Thu, Nov 01, 2018 at 07:36:12AM -0700, jrmarsha wrote:
@felix-salfelder What is your status on this?
tried to follow your instructions (rebase etc.). thought it worked.
(short on time.) what else can I do?
|
|
It really is just rebasing on boostorg/develop . |
felix-salfelder
force-pushed
the
bucket_sorter
branch
from
7b8de14 to
03573b4
Compare
|
On Sat, Jan 26, 2019 at 04:45:40PM -0800, Peter Dimov wrote:
No, you can't create an iterator from a pointer, and this has nothing
to do with Apple. https://godbolt.org/z/BEZSXf
try std::vector<int>::iterator i(p); in this godbolt thing, if you
trust it.
But that's not the original error. I suspect this pointer thing is a
later change. It shouldn't work anywhere.
of course it does. the issue is that a preprocessor workaround (not
mine) hacks around another apple issue. it then fails when trying to
assign.
i reverted the head type, lets see.
|
|
When you try to create an But that doesn't make it portable. |
there is no need or intent to handle graphs case by case. this partially reverts df1d527
fingers crossed.
signed unsigned comparison
felix-salfelder
force-pushed
the
bucket_sorter
branch
from
5c67d1f to
e62d657
Compare
|
On Sat, Jan 26, 2019 at 05:58:12PM -0800, Marshall Clow wrote:
But that doesn't make it portable.
thanks. now, it seems to be more portable. travis is happy with it, at
least.
|
|
any clues what might be wrong with msvc? the output is a bit cryptic, but it says "Assertion failed: vector iterator not dereferencable" in one of the logs... thanks |
|
@felix-salfelder, I really appreciate how long you persisted with this bug fix. |
|
On Sun, Mar 06, 2022 at 03:26:32PM -0800, Jeremy W. Murphy wrote:
@felix-salfelder, I really appreciate how long you persisted with this bug fix.
We had some issues with the CI and a lack of maintainers for a while but now things are (slowly) moving ahead.
If you have time to resolve the conflicts, then I'll look at it again, although not in the next week or so.
I have the magical maintainer hat now, so I can actually merge it in when it's ready!
Thanks for keeping up. Yes it's been a while...
Lots of whitespace conflicts, but not much else. I think a rebase will
be easier and less messy, would that be acceptable?
(No worries -- It won't happen this week anyway.)
Why has the bucket_sorter been classified "pending" in the first place?
Is there another issue with it that I have missed?
|
Sure, whatever is easier for you. It's been so long that I looked at this in any detail that I will essentially be looking at it afresh anyway. I don't actually know the history of the "pending" stuff. I guess it was for data structures that they thought should be moved into separate, more general-purpose Boost libraries. |
|
On Mon, Mar 07, 2022 at 02:32:31PM -0800, Jeremy W. Murphy wrote:
Sure, whatever is easier for you. It's been so long that I looked at
this in any detail that I will essentially be looking at it afresh
anyway.
I ended up doing it manually (whitespace all over the place), but I kept
the history, just in case I got it wrong.
The merge commit looks messy. But old and new diff actually should be
identical.
The old diff
git diff -w c583bfa...e62d657
The new diff
git diff -w master...bucket_sorter
|
felix-salfelder
force-pushed
the
bucket_sorter
branch
from
f900088 to
ab1689b
Compare
This is not a merge, which is completely impossible due to whitespace changes. Instead, manually applying the patch produced by $ git diff -w c583bfa...e62d657 and make it look like a merge.
felix-salfelder
force-pushed
the
bucket_sorter
branch
2 times, most recently
from
cd58f6b to
fd20cf6
Compare
|
Several checks fail -- any idea why? |
felix-salfelder
force-pushed
the
bucket_sorter
branch
from
fd20cf6 to
131c3ea
Compare
|
On Wed, Mar 16, 2022 at 03:46:31PM -0700, Jeremy W. Murphy wrote:
Several checks fail -- any idea why?
Yes, my bad. Leftover commas & stuff.
Worth a couple more cycles before I give up..
|
|
Thanks. I think the current CI failures are unrelated to your changes, although I'll have to confirm that before merging anything. |
|
On Sun, Mar 20, 2022 at 05:01:12PM -0700, Jeremy W. Murphy wrote:
Thanks. I think the current CI failures are unrelated to your changes,
although I'll have to confirm that before merging anything.
OK, there is no rush. Please do not change whitespace again, while this
might need another rebase..
|
removing an element from a stack does not always work, since the
predecessor of a stack element is not (always) stored. this patch
merges the array head into next, so a top element in a stack can point
to the head of the stack it is in.
remarks:
for elements deeper down in a stack
differences. (yes it's a bit ugly.)
enough.
test case. B is a bucketsorter operating on a vector V.
V[0]=0;
V[1]=1;
B.push(0);
B.push(1);
// now, stacks 0 and 1 are singletons.
// try to move 0 to stack 1, should result in
// head_1->0->1->end, but calls remove first, then
V[0]=1;
B.update(0); // <- BOOM
// the update calls remove, it "removes" 0 from stack V[0]=1.
// it's not there yet (!).
// instead 1 (top of bucket 1) must die.
// the result is head_1->0->end, and wrong.