-
Notifications
You must be signed in to change notification settings - Fork 176
feat: Add {Expr,Series}.sin
#3365
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
base: main
Are you sure you want to change the base?
Conversation
narwhals/_compliant/expr.py
Outdated
| def last(self) -> Self: ... | ||
| def skew(self) -> Self: ... | ||
| def kurtosis(self) -> Self: ... | ||
| def sin(self) -> Self: ... |
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.
@liamholmes31 I see that you removed this, but it was on the right track!
Do you mind if I add a commit with where this should be defined?
The file isn't in the changes so I couldn't use a suggestion
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.
sure, thanks @dangotbanned !
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 excuse the typo 🤦♂️
fix: Add ComplianColumn.sin
We should probably document this better!
All the methods that Expr and Series share and use Self can be added to CompliantColumn
narwhals/narwhals/_compliant/column.py
Lines 31 to 32 in 8d93fb1
| class CompliantColumn(Protocol): | |
| """Common parts of `Expr`, `Series`.""" |
So for example, CompliantColumn.log is here, since it works elementwise:
narwhals/narwhals/_compliant/column.py
Line 111 in 8d93fb1
| def log(self, base: float) -> Self: ... |
But for something like first, we need two separate definitions:
Here we aggregate to a literal value
narwhals/narwhals/_compliant/series.py
Line 137 in 8d93fb1
| def first(self) -> PythonLiteral: ... |
But here the aggregation is a literal expression
narwhals/narwhals/_compliant/expr.py
Line 141 in 8d93fb1
| def first(self) -> Self: ... |
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.
ahh OK this makes sense, thanks!! I was thrown by not spotting where log was defined in compliant, should have looked a bit harder!
|
might need some advice on the failing type check CI stage (see below), looks like the type hint for 'pc_func' may need to be relaxed? |
|
some of the failing downstream stages don't look obviously related to my changes here, are these known failing steps? |
|
Hey @liamholmes31 thanks for you PR 🚀 Looking forward to finalize it
They surely aren't related with the changes, we will address them separately or report the issues downstream 😉 |
looks like an issue with pyarrow-stubs, i'd suggest just type-ignoring (and ideally reporting upstream) |
Description
Added sin expr/series methods.
Have tried to follow existing PRs adding e.g. log methods - was a bit hazy on which classes in _compliant I needed to add these to, so apologies if anything is missing!
What type of PR is this? (check all applicable)
Related issues
Checklist