refactor: *(Namespace|DataFrame).from_numpy#2283
Conversation
`to_numpy` is already handled via `__getattr__`
Don't need to explicitly define in `CompliantDataFrame`, as it gets it from `NumpyConvertible`
The module already has a top-level `pl` import
- I thought what I had was allowed - but caused a runtime error in subclasses that *didn't* fill the missing `FromNumpyDT_contra` > TypeError: Too few arguments for <class 'narwhals._compliant.dataframe.CompliantDataFrame'>; actual 3, expected at least 4
One step towards `CompliantNamespace.from_numpy`
Adding the annotation of `CompliantNamespace` revealed a lot of gaps
Towards fixing this error, but without subclassing
```py
narwhals/utils.py:363: error: Incompatible return value type (got "PolarsNamespace", expected "CompliantNamespace[Any, Any]") [return-value]
return PolarsNamespace(backend_version=backend_version, version=version)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
narwhals/utils.py:363: note: Following member(s) of "PolarsNamespace" have conflicts:
narwhals/utils.py:363: note: selectors: expected "CompliantSelectorNamespace[Any, Any]", got "PolarsSelectors"
```
See note on property for blockers
Now we've got access to both `from_numpy` constructors from `Implementation`!
- Important to this PR is `is_eager_allowed` - Allows us to match `PolarsNamespace`, without defining redundant stuff in protocols/classes
- Spent too long trying to solve this - `pyright` has no issues - I can see moving `_to_compliant_namespace` into a function working - But I don't want that, and seems like a bug
Resolves #2283 (comment)
This part is independent of backends following (50a39df)
- All of that is functionally the same as what is covered - `_series.from_numpy` will get used in upcoming PRs https://github.com/narwhals-dev/narwhals/actions/runs/14045870768/job/39326463172?pr=2283
dangotbanned
changed the title
*(Namespace|DataFrame).from_numpy*(Namespace|DataFrame).from_numpy
|
The longer-term goal would be to transition more of We might already have everything in place for |
Thanks @MarcoGorelli Will investigate in #2283 (comment) |
dangotbanned
changed the title
*(Namespace|DataFrame).from_numpy*(Namespace|DataFrame).from_numpy
| def concat( | ||
| self: Self, | ||
| items: Iterable[PolarsLazyFrame], | ||
| *, | ||
| how: Literal["vertical", "horizontal", "diagonal"], | ||
| ) -> PolarsLazyFrame: ... | ||
|
|
||
| def concat( | ||
| self: Self, | ||
| items: Iterable[PolarsDataFrame] | Iterable[PolarsLazyFrame], | ||
| items: Iterable[FrameT], | ||
| *, | ||
| how: Literal["vertical", "horizontal", "diagonal"], | ||
| ) -> PolarsDataFrame | PolarsLazyFrame: |
There was a problem hiding this comment.
Resolves (#2283 (comment)) via (0e67a1e)
@MarcoGorelli
I had no idea a TypeVar could be used this way 🤔
One rule I thought was that a TypeVar on a parameter annotation must either:
- Be present in the class def (e.g.
Generic[T]orProtocol[T]) - Or appear in the return type
narwhals/narwhals/_polars/typing.py
Line 21 in 4f1b172
FrameT doesn't meet either of those rules - so I'd have expected this to get rejected.
But why work??
My best guess is there seems to be an understanding of what FrameT represents - and how that is consistent with:
narwhals/narwhals/_compliant/namespace.py
Lines 72 to 77 in 4f1b172
narwhals/narwhals/_compliant/typing.py
Line 74 in 4f1b172
narwhals/narwhals/_compliant/typing.py
Lines 40 to 42 in 4f1b172
Follow up to #2283 (comment)
2 participants
What type of PR is this? (check all applicable)
Related issues
CompliantSeries.from_numpy#2196 (comment)Checklist
If you have comments or can explain your changes, please do so below
Compliant*protocols #2230)from_numpyas all the supported backends already had ato_numpy, to complete theNumpyConvertiblepairfunctions.pypyright, butmypyhad issues with definingoverload(s) onImplementationnw.Namespaceclass to provide a scope for the singleImplementation->CompliantNamespacecontext