narwhals icon indicating copy to clipboard operation
narwhals copied to clipboard

Added docstring for concat

Open Sherwin-14 opened this issue 1 year ago • 4 comments

I wrote the docstring's for concat function. Please have a look into it

Sherwin-14 avatar Aug 18 '24 10:08 Sherwin-14

Thanks for the feedback, Marc! The CI tests seem to be failing though and I can't figure it out why, any clues?

Sherwin-14 avatar Aug 18 '24 11:08 Sherwin-14

  • df_pd_1

Hey, needed some clarification over this. I have changed both the data's to data_1 and data_2 but I am confused as to how we can change df to df_pd_1 or df_pl_1 ?

Sherwin-14 avatar Aug 18 '24 11:08 Sherwin-14

I have changed both the data's to data_1 and data_2 but I am confused as to how we can change df to df_pd_1 or df_pl_1 ?

you can do

data_1 = ...
data_2 = ...
df_pd_1 = pd.DataFrame(data_1)
df_pd_2 = pd.DataFrame(data_2)
df_pl_1 = pl.DataFrame(data_1)
df_pl_2 = pl.DataFrame(data_2)

MarcoGorelli avatar Aug 18 '24 12:08 MarcoGorelli

one more thing (as ci is failing)

can you update the docstring for concat in narwhals/stable/v1 as well please? it should be the same, but with import narwhals.stable.v1 as nw instead of import narwhals as nw

Hey I checked the narwhals/stable/v1 file and I did not find concat function there

Sherwin-14 avatar Aug 21 '24 08:08 Sherwin-14

one more thing (as ci is failing) can you update the docstring for concat in narwhals/stable/v1 as well please? it should be the same, but with import narwhals.stable.v1 as nw instead of import narwhals as nw

Hey I checked the narwhals/stable/v1 file and I did not find concat function there

Hey did you find some solution for this? the ci is still failing and I didn't find the concat function in stable/v1/.

Sherwin-14 avatar Aug 25 '24 10:08 Sherwin-14

Hey @Sherwin-14 , sorry I missed the first comment among various notifications.

I now see the issue, we somehow have the concat in stable/v1.py out of sync, or even worse, not even _stableify-ed yet. @MarcoGorelli I think we can do that in another PR as it seems a bit out of scope for this one?

FBruzzesi avatar Aug 25 '24 17:08 FBruzzesi

thanks for the ping, and sorry for the delay

in v1.py, concat currently just gets imported and re-exported:

from narwhals.functions import concat

so yes, maybe a separate PR to stableify concat would be good, then we can align the docstrings👍

MarcoGorelli avatar Aug 25 '24 17:08 MarcoGorelli

No worries. I can do this but what exactly needs to be done here? If you can outline the steps to me it will be a little bit easier for me.

Sherwin-14 avatar Aug 25 '24 17:08 Sherwin-14

thanks!

you'll need to do something like

diff --git a/narwhals/stable/v1.py b/narwhals/stable/v1.py
index 9c7c2d15..d96f0523 100644
--- a/narwhals/stable/v1.py
+++ b/narwhals/stable/v1.py
@@ -37,7 +37,7 @@ from narwhals.expr import Expr as NwExpr
 from narwhals.expr import Then as NwThen
 from narwhals.expr import When as NwWhen
 from narwhals.expr import when as nw_when
-from narwhals.functions import concat
+from narwhals.functions import concat as nw_concat
 from narwhals.functions import show_versions
 from narwhals.schema import Schema as NwSchema
 from narwhals.series import Series as NwSeries
@@ -1668,6 +1668,9 @@ def from_dict(
         nw.from_dict(data, schema=schema, native_namespace=native_namespace)
     )
 
+def concat(*args, **kwargs):
+    return _stableify(nw_concat(*args, **kwargs))
+

but instead of *args and **kwargs, put the actual variable names from the concat signature

MarcoGorelli avatar Aug 25 '24 17:08 MarcoGorelli

looks like Francesco's already on it in #869 though ⚡

MarcoGorelli avatar Aug 25 '24 18:08 MarcoGorelli

Hey @Sherwin-14 , we just merged a PR that adds concat function in stable/v1.py. Now you should be able to proceed with this PR after merging main branch

FBruzzesi avatar Aug 26 '24 17:08 FBruzzesi

Thanks a lot. I will do the needful :)

Sherwin-14 avatar Aug 27 '24 17:08 Sherwin-14

thanks @Sherwin-14 ! i think maybe something went wrong when merging, as docs/api-completeness.md is being shown as modified

MarcoGorelli avatar Sep 01 '24 10:09 MarcoGorelli

thanks @Sherwin-14 ! i think maybe something went wrong when merging, as docs/api-completeness.md is being shown as modified

Yeah I fixed that you can take a look at it now :)

Sherwin-14 avatar Sep 01 '24 10:09 Sherwin-14