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.
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
feat: make plotly-express dataframe agnostic via narwhals #4790
feat: make plotly-express dataframe agnostic via narwhals #4790
Changes from 57 commits
9873e97
0389591
ba93236
421fc1d
ca5c820
a6aab24
7665f10
ec4f250
7e0d4c2
5543638
cd0dab7
f334b32
e5eb949
7747e30
ac00b36
da80c5b
8a72ba1
aeff203
2041bef
71473f1
9f74c38
28587c9
1bb2448
f45addf
5341759
dfc957c
90f2667
fb58d1b
ddb3b35
37ce302
4da8768
210e01a
c00525e
3486a3e
c0ce093
0eb6951
844a6a9
0c27789
0e6ff78
c2337c9
2cc5d7b
1b27487
23a23be
7968cff
cf76721
91db84b
5c6772e
9ec3f9e
400a624
1aa5163
594ded0
6676061
d7d2884
d6ee676
82c114d
0ceabc1
c9b626e
87841d1
ffa7b3b
3ba19ae
a70146b
1fa9fe4
0103aa6
673d141
b858ed8
bbcf438
49efae2
a36bc24
c119153
7416407
1867f6f
d3a28c0
e6e9994
64b8c70
3f6b383
755aea8
6f18021
4d62e73
a770fd8
7d6f7d6
b8c10ec
490b64a
f7fd4c9
8753acb
1429e6f
878d4db
192e0a8
de6761c
bcfef68
519cc68
e5520a7
b855352
51e2b23
7ef9f28
e9a367d
12fed31
27b2996
f27f959
126a79d
7735366
b6516b4
6f1389f
db22268
b514c01
ce8fb9a
e47827e
9a9283a
2630a5a
fef6dbe
48c7f62
18cc11c
d94cbf7
c320c46
afdb31f
68ab52a
b8ccec4
f102998
2df0427
55a0178
bb327d5
7d611fb
a22a7be
44a52e5
fc74b2e
499e2fa
d2e1008
742b2ec
269dea6
b1dc48d
17fb96f
9f2c55b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
@MarcoGorelli for old numpy versions, arrow arrays and polars series
to_numpy
seem to be raisewhen then it is used by pandas operations.
I wonder if we
shouldcan allowto_numpy(..., writable: bool)
in narwhals to return a writable copyThere 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.
For my own curiosity why do we need Pandas still here? Does Narwhals provide a
series
equivalent?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.
We do not support
rolling
,expanding
norewm
at the current stage (see next line TODO comment).If and when that will be the case, then pandas can be completely optional.
Other way could be to implement these function in pure numpy, but I would rather keep it out of this PR even if that's an option. WDYT?
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.
I don't think there's a way to implement rolling functions in numpy, especially with some of the extra options
We'll add them to Narwhals though, we'd talked about it last month but decided that at the time it wasn't the top priority - but it is on the roadmap
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.
Got it. This approach makes sense to me -- I opened #4834 to keep track of this as a follow-up.
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.
rather than an option, does it makes sense here to check
y.flags['WRITEABLE']
, and copy if it's false?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.
What if the constructor functions were modified to accept either a Pandas df or a dict? Then if a Pandas df is passed it could call
.to_dict()
on it. Would save a bit of boilerplate in the tests.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.
Yes it makes sense. However let me mention that after our meeting today I started working to allow data(set) module to be dataframe agnostic. Different PR should be ready tomorrow and it may be a quick win to simplify data loading in tests as well. I will ping you there