Skip to content

docs: docstring for concat #808

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

Merged
merged 4 commits into from
Sep 1, 2024
Merged

Conversation

Sherwin-14
Copy link
Contributor

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

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Aug 18, 2024
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice one @Sherwin-14 , thanks!

a few requests:

  • could we make data_1, data_2 please? and then df_pd_1, df_pd_2, df_pl_1, df_pl_2?
  • could you check your statement about Series? I don't think we support them

@Sherwin-14
Copy link
Contributor Author

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
Copy link
Contributor Author

Sherwin-14 commented Aug 18, 2024

  • 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 ?

Copy link
Member

@FBruzzesi FBruzzesi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I have only one comment in addition to what Marco already mentioned.

Regarding the test failing, that's because we compare the exposed functions with what we expose in /stable/v1.py. I think this was not failing before because the docstring was empty. Now it should be added.

Here a hint on how to:

def concat(
    items: Iterable[FrameT],
    *,
    how: Literal["horizontal", "vertical"] = "vertical",
) -> FrameT:
    # Same docstring but using 
    # import narwhals.stable.v1 as nw 
    # inplace of import narwhals as nw
    
    return _stableify(nw.concat(items, how=how))

│ 5 ┆ 1 │
│ 2 ┆ 4 │
└─────┴─────┘
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding an example for horizontal concat as well?

@MarcoGorelli
Copy link
Member

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)

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for updating!

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice one, thanks @Sherwin-14 !

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Sherwin-14
Copy link
Contributor Author

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
Copy link
Contributor Author

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/.

@FBruzzesi
Copy link
Member

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 FBruzzesi changed the title Added docstring for concat docs: docstring for concat Aug 25, 2024
@MarcoGorelli
Copy link
Member

MarcoGorelli commented Aug 25, 2024

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👍

@Sherwin-14
Copy link
Contributor Author

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.

@FBruzzesi FBruzzesi mentioned this pull request Aug 25, 2024
10 tasks
@MarcoGorelli
Copy link
Member

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
Copy link
Member

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

@FBruzzesi
Copy link
Member

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

@Sherwin-14
Copy link
Contributor Author

Thanks a lot. I will do the needful :)

@MarcoGorelli
Copy link
Member

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

@Sherwin-14
Copy link
Contributor Author

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 :)

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is really good - thanks @Sherwin-14 , and @FBruzzesi for review!

@Sherwin-14 Sherwin-14 closed this Sep 1, 2024
@Sherwin-14 Sherwin-14 reopened this Sep 1, 2024
@MarcoGorelli MarcoGorelli merged commit a1c4289 into narwhals-dev:main Sep 1, 2024
41 checks passed
@InessaPawson InessaPawson linked an issue Sep 1, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOCS: add docstring for concat
4 participants