-
Notifications
You must be signed in to change notification settings - Fork 75
Cum sum fixes #1467
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
Cum sum fixes #1467
Conversation
* `cumSum` only works on columns that contain solely primitive numbers. | ||
* | ||
* @param [skipNA\] Whether to skip null and NaN values (default: `true`). | ||
* Similar to [sum][sum], [Byte][Byte]- and [Short][Short]-columns are converted to [Int][Int]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sum-sum! Bye-Bye-Bye!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, unfortunately, there are KDoc rendering issues if you don't alias the types here
* {@set [CumSumDocs.CUMSUM_PARAM]} | ||
*/ | ||
@Refine | ||
@Interpretable("GroupByCumSum0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are marking the functions for Compiler Plugin, should we add a test for this trivial case?
@koperagen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testing what? and how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, usually i simply annotate functions and later adjust if anything is wrong (one time i forgot Refine annotation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test
And perform interactive rebase + force push before merge so fixup! commits don't end up in the master
87a2998
to
8c5af58
Compare
48c9644
to
c8af8d1
Compare
some bug fixes and slight improvements required for #1061.
Also expanded kdoc