Skip to content

function barrier in npzreadarray and introduce readheader - #44

Merged
fhs merged 5 commits into
fhs:masterfrom
jishnub:header
Jan 9, 2021
Merged

function barrier in npzreadarray and introduce readheader#44
fhs merged 5 commits into
fhs:masterfrom
jishnub:header

Conversation

@jishnub

@jishnub jishnub commented Jan 5, 2021

Copy link
Copy Markdown
Contributor

This PR does two things:

  • Make the Header type parametric and concrete: this seems to cut down on allocations while reading arrays

On master:

julia> npzwrite("abc.npy", ones(3,4));

julia> @btime npzread("abc.npy");
  23.639 μs (91 allocations: 5.80 KiB)

After this PR:

julia> @btime npzread("abc.npy");
  20.911 μs (80 allocations: 5.28 KiB)

The difference in run-time is marginal.

  • Introduce a function readheader that returns the header. This might be convenient to get the eltype and size of the array without reading it in (eg. to pre-allocate an array of the correct size). This function is not exported.
julia> npzwrite("abc.npy", ones(3,4));

julia> NPZ.readheader("abc.npy")
NPZ.Header{Float64,2,typeof(ltoh)}(ltoh, true, (3, 4))

julia> NPZ.readheader("abc.npy") |> size
(3, 4)

julia> NPZ.readheader("abc.npy") |> eltype
Float64

@codecov-io

codecov-io commented Jan 5, 2021

Copy link
Copy Markdown

Codecov Report

Merging #44 (37374e4) into master (9a66947) will increase coverage by 0.85%.
The diff coverage is 92.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
+ Coverage   85.29%   86.15%   +0.85%     
==========================================
  Files           1        1              
  Lines         170      195      +25     
==========================================
+ Hits          145      168      +23     
- Misses         25       27       +2     
Impacted Files Coverage Δ
src/NPZ.jl 86.15% <92.10%> (+0.85%) ⬆️

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 9a66947...37374e4. Read the comment docs.

@fhs fhs left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks!

It looks ok from a high level. I didn't look at it in detail -- I stopped using Julia years ago and I've lost a lot of my knowledge and also Julia has changed. I'm looking for other people to help review these PRs.

@fhs
fhs merged commit 8834d3c into fhs:master Jan 9, 2021
@jishnub
jishnub deleted the header branch January 9, 2021 05:43
@jishnub

jishnub commented Jan 9, 2021

Copy link
Copy Markdown
Contributor Author

Thank you for your package! Could you tag a new version by commenting @JuliaRegistrator register on the merged commit?

@JuliaRegistrator

Copy link
Copy Markdown

Error while trying to register: Register Failed
@jishnub, it looks like you don't have collaborator status on this repository.

@fhs

fhs commented Jan 9, 2021

Copy link
Copy Markdown
Owner

Thank you for your package! Could you tag a new version by commenting @JuliaRegistrator register on the merged commit?

Ok, commented

@JuliaRegistrator

Copy link
Copy Markdown

Comments on pull requests will not trigger Registrator, as it is disabled. Please try commenting on a commit or issue.

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.

4 participants