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

Bump ChunkSplitters to 3.0 #40

Merged
merged 5 commits into from
Oct 20, 2024
Merged

Bump ChunkSplitters to 3.0 #40

merged 5 commits into from
Oct 20, 2024

Conversation

henry2004y
Copy link
Contributor

ChunkSplitters.jl 3.0 introduces a breaking change to the chunks API, where n (the number of chunks) is now a keyword parameter.

@DanielVandH
Copy link
Owner

Thanks for this. I forgot there was a new release for that package. From https://github.com/JuliaFolds2/ChunkSplitters.jl/blob/main/CHANGELOG.md, I think instead the fix is to change chunks to index_chunks instead? chunks is a different function now apparently...

@henry2004y
Copy link
Contributor Author

henry2004y commented Oct 19, 2024

Thanks for this. I forgot there was a new release for that package. From https://github.com/JuliaFolds2/ChunkSplitters.jl/blob/main/CHANGELOG.md, I think instead the fix is to change chunks to index_chunks instead? chunks is a different function now apparently...

Yeah you are right, I forgot about that. Maybe in some places where the indices are not needed, we can simply use chunks. I just replace all chunks with index_chunks for now.


Hmm, I have no idea why the tests are failing. It seems that probably in some tests there are only one element with two threads?

@DanielVandH
Copy link
Owner

DanielVandH commented Oct 20, 2024

Do they fail with v2? You probably need to look at what index_chunks is actually giving for these vectors compared to v2's chunks

@henry2004y
Copy link
Contributor Author

henry2004y commented Oct 20, 2024

OK, I now understand the issue. We need to add enumerate as well as switching the order of indices and values. Actually the new way follows the natural argument order returned from enumerate, which is good. I didn't run the tests on my laptop because it took quite a while :)

Copy link

codecov bot commented Oct 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.86%. Comparing base (1fb57cb) to head (403ebb4).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
- Coverage   97.86%   97.86%   -0.01%     
==========================================
  Files          22       21       -1     
  Lines        1216     1215       -1     
==========================================
- Hits         1190     1189       -1     
  Misses         26       26              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DanielVandH
Copy link
Owner

Thanks for looking into this! I appreciate it. If you can remove the vscode settings file and bump the package version I'll merge and make a new release

@DanielVandH
Copy link
Owner

I didn't run the tests on my laptop because it took quite a while :)

And yeah they take a bit of time unfortunately, sorry about that.. probably longer than is needed but back when I was first developing the package it helped to be a bit exhaustive 😅

@DanielVandH DanielVandH merged commit 24be1a0 into DanielVandH:main Oct 20, 2024
3 of 4 checks passed
@DanielVandH
Copy link
Owner

New version has now been released. Thanks @henry2004y!

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.

2 participants