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.

Sign up for GitHub

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

[BUG] Make name attr of Index fast slow attrs #16270

Merged
merged 12 commits into from
Jul 16, 2024
Merged

[BUG] Make name attr of Index fast slow attrs #16270

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e20356c
Initial commit
Matt711 Jul 9, 2024
515e19e
Merge branch 'branch-24.08' of github.com:rapidsai/cudf into bug/inde…
Matt711 Jul 10, 2024
604d107
Add a test
Matt711 Jul 10, 2024
3604ce3
Fix typo xpd not pd
Matt711 Jul 10, 2024
b019868
Merge branch 'branch-24.08' of github.com:rapidsai/cudf into bug/inde…
Matt711 Jul 12, 2024
3ad0081
Fix index names
Matt711 Jul 12, 2024
e741b72
Change _name to name
Matt711 Jul 12, 2024
38c3b78
Missed one
Matt711 Jul 12, 2024
2bc8427
Merge branch 'branch-24.08' into bug/fix-index-name
galipremsagar Jul 12, 2024
e06eacd
remove __setattr__
Matt711 Jul 12, 2024
21b7126
Merge branch 'bug/fix-index-name' of github.com:Matt711/cudf into bug…
Matt711 Jul 12, 2024
e71ec51
revert
Matt711 Jul 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 14 additions & 22 deletions python/cudf/cudf/pandas/_wrappers/pandas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,18 +260,14 @@ def Index__new__(cls, *args, **kwargs):
return self


def name(self):
return self._fsproxy_wrapped._name


def Index__setattr__(self, name, value):
Copy link
Contributor

Choose a reason for hiding this comment

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

With the usage of _FastSlowAttribute I don't think we need Index__setattr__ anymore.

Copy link
Contributor Author

@Matt711 Matt711 Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, except for MultiIndex.

edit: It might need some more customization because it doesn't just yet without Index__setattr__

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't defining names Fastslow attribute for MultiIndex just do the job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

names attr in Pandas is set with property like this. I think_set_names is not being picked up for some reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pd.MultiIndex([]).names = ... isn't doing anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I think we still need Index__setattr__ in Index types. This issue address the problem described here.

Copy link
Contributor

Choose a reason for hiding this comment

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

No I think we still need Index__setattr__ in Index types. This issue address the problem described here.

I feel if we do this for one attribute we will have to end up doing the same for every attribute, which is why I think there could be a better approach to solve this problem. Let's pair on this tomorrow ?

if name.startswith("_"):
object.__setattr__(self, name, value)
return
if name == "name":
setattr(self._fsproxy_wrapped, "_name", value)
setattr(self._fsproxy_wrapped, "name", value)
if name == "names":
setattr(self._fsproxy_wrapped, "_names", value)
setattr(self._fsproxy_wrapped, "names", value)
return _FastSlowAttribute("__setattr__").__get__(self, type(self))(
name, value
)
Expand Down Expand Up @@ -300,7 +296,7 @@ def Index__setattr__(self, name, value):
"_accessors": set(),
"_data": _FastSlowAttribute("_data", private=True),
"_mask": _FastSlowAttribute("_mask", private=True),
"name": property(name),
"name": _FastSlowAttribute("name"),
},
)

Expand All @@ -314,7 +310,7 @@ def Index__setattr__(self, name, value):
additional_attributes={
"__init__": _DELETE,
"__setattr__": Index__setattr__,
"name": property(name),
"name": _FastSlowAttribute("name"),
},
)

Expand Down Expand Up @@ -345,7 +341,7 @@ def Index__setattr__(self, name, value):
additional_attributes={
"__init__": _DELETE,
"__setattr__": Index__setattr__,
"name": property(name),
"name": _FastSlowAttribute("name"),
},
)

Expand Down Expand Up @@ -375,10 +371,10 @@ def Index__setattr__(self, name, value):
bases=(Index,),
additional_attributes={
"__init__": _DELETE,
"__setattr__": Index__setattr__,
"_data": _FastSlowAttribute("_data", private=True),
"_mask": _FastSlowAttribute("_mask", private=True),
"__setattr__": Index__setattr__,
"name": property(name),
"name": _FastSlowAttribute("name"),
},
)

Expand Down Expand Up @@ -412,10 +408,10 @@ def Index__setattr__(self, name, value):
bases=(Index,),
additional_attributes={
"__init__": _DELETE,
"__setattr__": Index__setattr__,
"_data": _FastSlowAttribute("_data", private=True),
"_mask": _FastSlowAttribute("_mask", private=True),
"__setattr__": Index__setattr__,
"name": property(name),
"name": _FastSlowAttribute("name"),
},
)

Expand Down Expand Up @@ -470,10 +466,10 @@ def Index__setattr__(self, name, value):
bases=(Index,),
additional_attributes={
"__init__": _DELETE,
"__setattr__": Index__setattr__,
"_data": _FastSlowAttribute("_data", private=True),
"_mask": _FastSlowAttribute("_mask", private=True),
"__setattr__": Index__setattr__,
"name": property(name),
"name": _FastSlowAttribute("name"),
},
)

Expand Down Expand Up @@ -508,10 +504,6 @@ def Index__setattr__(self, name, value):
)


def names(self):
return self._fsproxy_wrapped._names


MultiIndex = make_final_proxy_type(
"MultiIndex",
cudf.MultiIndex,
Expand All @@ -522,7 +514,7 @@ def names(self):
additional_attributes={
"__init__": _DELETE,
"__setattr__": Index__setattr__,
"name": property(names),
"names": _FastSlowAttribute("names"),
},
)

Expand Down Expand Up @@ -709,10 +701,10 @@ def names(self):
bases=(Index,),
additional_attributes={
"__init__": _DELETE,
"__setattr__": Index__setattr__,
"_data": _FastSlowAttribute("_data", private=True),
"_mask": _FastSlowAttribute("_mask", private=True),
"__setattr__": Index__setattr__,
"name": property(name),
"name": _FastSlowAttribute("name"),
},
)

Expand Down
Loading