Fix sort-axis example, add test to confirm original error and resolution #916
Conversation
|
I pushed (a fix to your branch), but I'd prefer if real tests were added before merge - we can find a shorter example that was wrong before. And we'd need to fix CI so that it actually tests the examples. |
| moved_elements += 1; | ||
| }); | ||
| } | ||
| Zip::from(&perm.indices) |
There was a problem hiding this comment.
tip, we can use slices just fine in Zip.
All that was needed for the fix was to swap perm_i and i, but I saw this change as an improvement.
There was a problem hiding this comment.
Thanks for the tip, I haven't used Zip much but should start. I noticed this change being beyond the index swap.
Ah, I made a comment above about the test being outdated now (before seeing this one), I will trim up the file to reproduce the original error and amend my last commit. Also added |
dam5h
force-pushed
the
permute-axis-not-sorting
branch
from
179f75d
to
2149428
Compare
|
Can we squash together the commits? In particular so that the big array file disappears from the history-to-merge. Thanks :) |
examples/sort-axis.rs
Outdated
|
|
||
| let perm = a.sort_axis_by(Axis(0), |i, j| a[[i, 0]] < a[[j, 0]]); | ||
| let b = a.permute_axis(Axis(0), &perm); | ||
| assert_eq!(b.row(0), array![75600.0927734375, 0.00011850080901105835]); |
There was a problem hiding this comment.
sorry for the nitpick but sorting is best by asserting that every element is <= its predecessor (and some way of checking the other elements in the same row are moved correctly).
There was a problem hiding this comment.
Agreed! I'll do this and then squash them up.
also replaced outer iteration with zip directly since it takes slices.
dam5h
force-pushed
the
permute-axis-not-sorting
branch
from
2149428
to
c27626a
Compare
|
Don't forget to update pr description & title before merge for the new state of the PR. Thanks for this |
dam5h
changed the title
|
Thanks for this! |
|
Sure, thanks for your help and the library! Used |
Noticed a sorting issue when using this example code in my application. After building a couple tests and sharing with @bluss , he determined that the
perm_iandivars were swapped in the Zip comprehension. Once fixed, a single test case was left which would have caught the original failure.