Feat: Series.shift Pyarrow Backend Implementation
#590
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
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…com/mfonekpo/narwhals into factor_out_testing_from_test_common
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…narwhals into Feat_Series_drop_nulls
FBruzzesi
reviewed
Jul 25, 2024
FBruzzesi
reviewed
Jul 25, 2024
|
Alright then.
…On Thu, 25 Jul 2024, 06:26 Francesco Bruzzesi, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In narwhals/_arrow/series.py
<#590 (comment)>
:
> + def shift(self, n: int) -> Self:
+ pc = get_pyarrow_compute()
+ return self._from_native_series(pc.shift(self._native_series, n))
This might end up being not so straightforward after all. We should
*manually* slice the original array and build the new one, this will also
be dependent on the sign of n
—
Reply to this email directly, view it on GitHub
<#590 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOA4GJDXMJF737IFUBBU7QTZOCEA7AVCNFSM6AAAAABLKD7LZSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJYGMZTSOJUGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
There is room for growth, right? Thank you. Is it ok? because you said it
is out of interest. What do I do?
…On Sun, Jul 28, 2024 at 2:09 PM Marco Edward Gorelli < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In narwhals/_arrow/series.py
<#590 (comment)>
:
> + # This condition shifts the series by n positions.
+ if n > 0:
+ # If n is positive, prepend nulls and truncate the end
+ shifted_chunks = [pa.array([None] * n, type=dtype)] + [
+ chunk.slice(0, len(chunk) - n) for chunk in chunks
+ ]
+ elif n < 0:
+ # If n is negative, append nulls and truncate the start
+ shifted_chunks = [chunk.slice(-n, len(chunk) + n) for chunk in chunks] + [
+ pa.array([None] * -n, type=dtype)
+ ]
+ else:
+ # If n is 0, return the original series
+ shifted_chunks = chunks
cool, thanks - I'm just a bit surprised, as it looks a bit more advanced
than other work you've submitted. if you figured this all out, then well
done 🙌
—
Reply to this email directly, view it on GitHub
<#590 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOA4GJGVBBK57PHKQ75XC43ZOTUQXAVCNFSM6AAAAABLKD7LZSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMBTGQ4DKOBYGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
yup, definitely room for growth :) it's OK, if you add the test I suggested and it passes, then it should be fine |
|
Thanks, I will!
…On Sun, Jul 28, 2024 at 2:56 PM Marco Edward Gorelli < ***@***.***> wrote:
yup, definitely room for growth :) it's OK, if you add the test I
suggested and it passes, then it should be fine
—
Reply to this email directly, view it on GitHub
<#590 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOA4GJGEKAN6PIUI3FGRY3DZOT2AZAVCNFSM6AAAAABLKD7LZSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGUZDOOJWGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
thanks for updating 🙏 ! have left a few comments
In general, I'd suggest that before asking for a review, you click on "files changed" and look through your changes yourself - it happens quite often that extra changes end up there which shouldn't be there. This fine is fine whilst learning 🤠 I'm just pointing it out so you know how to improve
tests/expr_and_series/shift_test.py
Outdated
Comment on lines
14
to
15
| def test_shift(constructor_eager: Any) -> None: | ||
| df = nw.from_native(constructor_eager(data), eager_only=True) |
There was a problem hiding this comment.
these don't need to change in this test - only in test_shift_series did you need to do that, constructor is fine here, because we're testing Expr.shift
Suggested change
| def test_shift(constructor_eager: Any) -> None: | |
| df = nw.from_native(constructor_eager(data), eager_only=True) | |
| def test_shift(constructor: Any) -> None: | |
| df = nw.from_native(constructor(data)) |
There was a problem hiding this comment.
could you address this comment please?
There was a problem hiding this comment.
This still needs addressing, could you read over the suggested change more carefully and address it please?
You've removed the eager_only part, but you still need to rename constructor_eager to constructor (just for this test!)
9 tasks
MarcoGorelli
approved these changes
Aug 7, 2024
There was a problem hiding this comment.
I've added a commit to get this through
A bit of general feedback:
- in tests, you should construct
expectedexplicitly, without any logic. The test case I handed you was like this, there was no need to modify it - the implementation for
shiftwasn't quite right. I'd pointed you to the solution (https://stackoverflow.com/a/78805975/4451315), why not try to just use that?
Anyway, thanks for your work here 💪 let's ship it
|
Thank you Marco, I have taken note of the points you listed. should I
refactor it to meet the standard?
…On Wed, Aug 7, 2024 at 2:45 PM Marco Edward Gorelli < ***@***.***> wrote:
***@***.**** approved this pull request.
I've added a commit to get this through
A bit of general feedback:
- in tests, you should construct expected explicitly, without any
logic. The test case I handed you was like this, there was no need to
modify it
- the implementation for shift wasn't quite right. I'd pointed you to
the solution (https://stackoverflow.com/a/78805975/4451315), why not
try to just use that?
Anyway, thanks for your work here 💪 let's ship it
<https://camo.githubusercontent.com/1b87ba81f294957cce02b6ad5cb2ab324f25046e1c706c96323f544aa20f81a4/68747470733a2f2f692e666c756666792e63632f716b4e667050306a325074647439764458327171334e435871646b634b3673532e676966>
—
Reply to this email directly, view it on GitHub
<#590 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOA4GJHV7XXD6FB2ZXONB7LZQIQH3AVCNFSM6AAAAABLKD7LZSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMRVGE3TMMBVGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
What type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below.