Skip to content

Conversation

@christiangnrd
Copy link
Member

This is essentially #38 for the rest of the package, as well as some opinionated (but non-functional) related changes.

Pros:

  • Reduce duplication of defaults arguments, potentially reducing future typo-related bugs.
  • Cleaner code for those familiar with Julia

Cons:

  • More coupling of code (although different defaults can easily be added where needed
  • Potentially harder to read for those unfamiliar with the code base.
  • I don't think it does, but I have not tested if this has any impact on performance

@anicusan I wrote this up to be helpful, none of this is necessary for my GSOC objectives. If you like it, great! If not, no problem, I'll just close this PR. I do think bringing down the "sources of truth" could help with preventing bugs from copy-paste or things like #37.

@anicusan
Copy link
Member

My only problem is that the kwargs... will silently accept typos, e.g. AK.foreachindex(..., max_threads=1) instead of max_tasks, then silently ignore unnecessary kwargs. It is a bit of duplication on our dev side, but it avoids unexpected behaviours for users - and shows useful errors in case of typos / wrong kwargs.

But internally - forwarding kwargs from our exposed functions to the internal implementations - I think this is a good idea, if you'd be okay with updating this PR (including the new multithreaded algorithms in 0.4.0 - sorry 😅)

@christiangnrd
Copy link
Member Author

It should error once it reaches the implementation. Maybe I can add a test for every public method to ensure that it properly throws an ArgumentError?

I'll rebase #38 since this PR depends on it, and once that's merged I'll take care of this one

@christiangnrd
Copy link
Member Author

@anicusan This is ready for final review. I probably shouldn't have force-pushed so I'll detail the changes I've made.

Changes mixed in with the force push:

  • Rebased on 0.4
  • Added tests for undefined kwargs

And then I also applied some of the same refactors into the new 0.4 files, and I removed _reduce_impl in favour of directly calling _mapreduce_impl(identity, ...) since after the kwarg changes it felt unecessary.

Please let me know before merging, I want to clean up the commit history beforehand.

@christiangnrd
Copy link
Member Author

oneAPI error seems like an upstream issue. I've encountered it before

@anicusan
Copy link
Member

This is great, thanks for also adding the MethodError tests. It's good to merge from my side - feel free to do any commit cleanup you'd like :)

@christiangnrd
Copy link
Member Author

Merge away!

@anicusan anicusan merged commit 92beead into JuliaGPU:main May 27, 2025
37 of 38 checks passed
@anicusan
Copy link
Member

Really appreciate this, @christiangnrd

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