Skip to content
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: add native property and deprecate .to_native() method #1301

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

FBruzzesi
Copy link
Member

What type of PR is this? (check all applicable)

  • πŸ’Ύ Refactor
  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ”§ Optimization
  • πŸ“ Documentation
  • βœ… Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below.

@github-actions github-actions bot added the enhancement New feature or request label Nov 1, 2024
narwhals/dataframe.py Outdated Show resolved Hide resolved
@MarcoGorelli
Copy link
Member

nice!

  • should we use this in all the public docs places where to_native currently appears?
  • I think we should have it such that in the main namespace, DataFrame.to_native / LazyFrame.to_native / Series.to_native issue deprecation warnings, but not in stable.v1 (could be a follow-up)
  • i think the same docstring as the current to_native functions would be good?

@FBruzzesi
Copy link
Member Author

I updated accordingly, let me leave a couple of comments:

  • should we use this in all the public docs places where to_native currently appears?

I replaced only the cases for .to_native() method (one instance), not nw.to_native(...)

  • I think we should have it such that in the main namespace, DataFrame.to_native / LazyFrame.to_native / Series.to_native issue deprecation warnings, but not in stable.v1 (could be a follow-up)

I added this, and tested as well. Yet, since it is a free operation, I don't think it is a big drawback keeping it. I personally like the to_native() api better (not sure why).

  • i think the same docstring as the current to_native functions would be good?

πŸ‘πŸΌ

@FBruzzesi
Copy link
Member Author

My understanding is that CI is failing because all the imports in stable v1 docstrings changed

+ import narwhals as nw
- import narwhals.stable.v1 as nw

how's that the case?

@MarcoGorelli
Copy link
Member

thanks!

I added this, and tested as well. Yet, since it is a free operation, I don't think it is a big drawback keeping it. I personally like the to_native() api better (not sure why).

πŸ€” maybe we can bikeshed on this longer then πŸ˜„ longterm I think it'd be better to stabilise on a single api...on the one hand, I like the symmetry of to/from, but on the other hand, I do like that .native makes it look more like it is (a pratically free operation)

@MarcoGorelli
Copy link
Member

My understanding is that CI is failing because all the imports in stable v1 docstrings changed

+ import narwhals as nw
- import narwhals.stable.v1 as nw

how's that the case?

sorry yeah this was in #1278

@FBruzzesi
Copy link
Member Author

sorry yeah this was in #1278

Just to be sure, was this intended?

@MarcoGorelli
Copy link
Member

CI looks like it's green now, I think the only failure is because of deprecation warnings having changed in py13, looking into it

@FBruzzesi
Copy link
Member Author

CI looks like it's green now, I think the only failure is because of deprecation warnings having changed in py13, looking into it

Found the doctest that was raising, I changed it to call .native instead, and I am skipping the .to_native() tests in stable api v1. Those would fail unless we change it back to the import import narwhals.stable.v1 as nw

@FBruzzesi FBruzzesi changed the title feat: add native property feat: add native property and depredate .to_native() method Nov 2, 2024
@FBruzzesi FBruzzesi changed the title feat: add native property and depredate .to_native() method feat: add native property and deprecate .to_native() method Nov 2, 2024
@MarcoGorelli
Copy link
Member

thanks!

I personally like the to_native() api better (not sure why).

shall we sleep on this one a bit longer then? i don't feel super-strongly about this one tbh, happy to keep to_native() if you prefer it

@FBruzzesi
Copy link
Member Author

I personally like the to_native() api better (not sure why).

Would it be bad to introduce .native without deprecating .to_native()? I understand it may be confusing for a user. Maybe changing the description of .to_native() to explicitly mention that's just an alias for .native and that's a free operation.

@EdAbati
Copy link
Contributor

EdAbati commented Nov 5, 2024

I also don't have a strong preference.
I see why .native hints that it is a 'free' operation but I kind of like how to_native reads πŸ˜… (maybe just because I got used to it)

We could keep it as alias for now, and move the discussion to an issue. In the meantime, we can suggest .native in every docs to signal it is the 'preferred way'.

@MarcoGorelli
Copy link
Member

maybe this can be a good topic for discussion for tomorrow's community call?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add DataFrame.native / LazyFrame.native / Series.native properties
3 participants