Skip to content

Conversation

bluss
Copy link
Member

@bluss bluss commented May 13, 2021

build_uninit, is necessary for abstracting over any kind of owned array here.

Fixes #277
Requires ndarray 0.15.2

@bluss bluss force-pushed the use-build-uninit branch from da65972 to 577f154 Compare May 13, 2021 09:12
Zip::indexed(&mut a).for_each(|(i, j), elt| {
if i > j { *elt = A::zero() }
});
a
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR has an inefficiency here since zero elements are overwritten after copying (not benchmarked, in some cases probably faster/just as fast because the indexed iterator is slow).

@bluss bluss force-pushed the use-build-uninit branch from 577f154 to dd594f7 Compare May 13, 2021 09:14
@bluss bluss marked this pull request as ready for review May 17, 2021 19:12
@bluss bluss force-pushed the use-build-uninit branch from dd594f7 to c3aa85a Compare May 17, 2021 19:14
@codecov
Copy link

codecov bot commented May 17, 2021

Codecov Report

Merging #287 (b0d8529) into master (a561e5a) will increase coverage by 0.00%.
The diff coverage is 93.33%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #287 +/- ## ======================================= Coverage 89.01% 89.01% ======================================= Files 71 71 Lines 3577 3578 +1 ======================================= + Hits 3184 3185 +1  Misses 393 393 
Impacted Files Coverage Δ
ndarray-linalg/src/convert.rs 95.74% <88.88%> (+0.09%) ⬆️
ndarray-linalg/src/qr.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a561e5a...b0d8529. Read the comment docs.

@bluss bluss force-pushed the use-build-uninit branch from c3aa85a to 461f234 Compare May 18, 2021 18:27
build_uninit is necessary for abstracting over any kind of owned array. The version of take_slice_upper is still inefficient in one way (overwriting after copying), but could likely still be an improvement upon the previous version.
Copy link
Member

@jturner314 jturner314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me. The inefficiency in take_slice_upper is fine for now, IMO. We can always benchmark and improve it later if needed.

@jturner314 jturner314 merged commit 6de9acc into master May 27, 2021
@jturner314
Copy link
Member

I merged this because it looks good to me, we really need to release a new version of ndarray-linalg, and I haven't heard from @termoshtt in a while.

@bluss bluss deleted the use-build-uninit branch May 27, 2021 22:05
termoshtt added a commit that referenced this pull request Jun 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants