Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port usage of uninitialized to build_uninit #287

Merged
merged 1 commit into from
May 27, 2021
Merged

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
let mut a = replicate(&av);
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace deprecated ArrayBase::uninitialized
2 participants