Refactor cudf.Series.__init__ #14450
Conversation
mroeschke
added
Python
python/cudf/cudf/core/series.py
Outdated
| name_from_data = data.name | ||
| column = as_column(data, nan_as_null=nan_as_null, dtype=dtype) | ||
| if isinstance(data, (pd.Series, Series)): | ||
| index, index_from_data = as_index(data.index), index |
There was a problem hiding this comment.
The naming is confusing for me on this line. index is the passed in argument, index_from_data is the index extracted from data. However it seems as if the meaning swapped on this line?
There was a problem hiding this comment.
Yeah I can see how this is confusing, I'll swap the naming here throughout the __init__ to make it clearer
python/cudf/cudf/core/series.py
Outdated
| data = data.astype(dtype) | ||
|
|
||
| if isinstance(data, dict): | ||
| elif isinstance(data, dict) or data is None: |
There was a problem hiding this comment.
Perhaps separating these two conditions as two branches is clearer?
There was a problem hiding this comment.
I can swap data=None for data={} earlier and simplify this condition
| if dtype is not None: | ||
| column = column.astype(dtype) |
There was a problem hiding this comment.
After passing dtype into every as_column call above, why do we still need to cast the column type here? Just curious!
There was a problem hiding this comment.
This is mostly defensive for now as I am not sure if as_column currently always respects dtype casting (e.g. I found a case recently in https://github.com/rapidsai/cudf/pull/14686/files) but I think this could be removed in the future!
python/cudf/cudf/core/series.py
Outdated
| if index_from_data is not None: | ||
| # TODO: This there a better way to do this? | ||
| index_from_data = as_index(index_from_data) | ||
| reindexed = self.reindex(index=index_from_data, copy=False) |
There was a problem hiding this comment.
There seems to be a _reindex function that can take inplace=True parameter.
There was a problem hiding this comment.
I just tried this and there were some test failures around data types not being preserved during the _reindex. I guess I'll just do this for now
|
/merge |
Description
I found a few issues related to reindexing and name priority when
index=andname=are given, so added unit tests for those.Additionally added some typing in
from_pandasmethodsThe main approach here is to collect the potential column from the
if/elifbranchs first, callsuper().__init__({name: columns}, index=index), and then apply the additional keywords to the resultChecklist