Address #37 #38
Merged
Address #37 #38
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also remove redundant `kwarg=kwarg` definitions for `accumulate`
All defaults removed in this commit are always overridden by the arguments passed in from `accumulate_impl!`. This removes any confusion of where a certain argument would have come from
Member
Author
|
@anicusan I rebased for 0.4, and I removed some default argument definitions for the deep implementation functions that would never be used and probably just add confusion. I've also added tests (for accumulate only in this PR) to ensure that invalid kwargs error. |
Member
|
This is great, thanks @christiangnrd ! The "opinionated cleanup" is also fantastic. Big fan of the |
Member
Author
|
Thank you! Would you be able to bump to 0.4.1 with this since it’s a also a bugfix? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'll give a better title once this isn't a prototype, but this is the fix I suggested in #37.
I split it into two commits: The first to fix the actual issue reported, and the second goes over the rest of
accumulateas a proof-of-concept.If this is approved, I can go over the whole package and simplify the the rest. This would probably allow us to remove theI did it anyway. See #39cumsumandcumproddefinitions in the Metal extension.