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

Extend chunk to take size as an argument #112

Merged
merged 4 commits into from
Jul 10, 2022

Conversation

theabhirath
Copy link
Contributor

@theabhirath theabhirath commented Jul 6, 2022

Should close #109. I would've left this to the author of that issue but I need this for some of the Metalhead.jl work on Res2Net.


This PR extends chunk to expect an optional kwarg size, which should return chunks of size size. I've rewritten the way chunk(x, n) works since that seemed like a natural extension and tests pass, but I'm not an expert at the rrule stuff so if there's something wrong feedback would be appreciated 😅

Copy link

@gruberchr gruberchr left a comment

Choose a reason for hiding this comment

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

Adding method chunk(x; size) looks good to me. A test for the new method is still missing. Or is it sufficient to test it implicitly via method chunk(x, n)?
Additionally I added some comments suggesting to use function cld() for simplification.
I'm not familiar with writing rrules as well, therefore I'm not able to check that.

src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
test/utils.jl Outdated Show resolved Hide resolved
Add tests for `chunk` with `size`
@theabhirath
Copy link
Contributor Author

Thanks for the comments! I've changed the code to use cld, and I've also added some tests with chunk taking in size

@codecov-commenter
Copy link

codecov-commenter commented Jul 7, 2022

Codecov Report

Merging #112 (1128724) into main (b29bf41) will decrease coverage by 0.18%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
- Coverage   88.55%   88.36%   -0.19%     
==========================================
  Files          13       13              
  Lines         533      533              
==========================================
- Hits          472      471       -1     
- Misses         61       62       +1     
Impacted Files Coverage Δ
src/utils.jl 90.24% <77.77%> (-0.82%) ⬇️
src/batchview.jl 80.85% <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 b29bf41...1128724. Read the comment docs.

@gruberchr
Copy link

👍

@theabhirath
Copy link
Contributor Author

Bump?

@ToucheSir ToucheSir merged commit 387b371 into JuliaML:main Jul 10, 2022
@theabhirath theabhirath deleted the chunk-size branch July 11, 2022 01:58
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.

Extend function chunk() to expect a chunk size parameter?
5 participants