Skip to content

STYLE: Use decorator syntax instead of legacy syntax for defining properties in Cython #18602

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

Merged
merged 1 commit into from
Dec 6, 2017
Merged

STYLE: Use decorator syntax instead of legacy syntax for defining properties in Cython #18602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
40 changes: 18 additions & 22 deletions pandas/_libs/index.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,34 +220,31 @@ cdef class IndexEngine:
def __sizeof__(self):
return self.sizeof()

property is_unique:
@property
def is_unique(self):
if self.need_unique_check:
self._do_unique_check()

def __get__(self):
if self.need_unique_check:
self._do_unique_check()

return self.unique == 1
return self.unique == 1

cdef inline _do_unique_check(self):

# this de-facto the same
self._ensure_mapping_populated()

property is_monotonic_increasing:

def __get__(self):
if self.need_monotonic_check:
self._do_monotonic_check()
@property
def is_monotonic_increasing(self):
if self.need_monotonic_check:
self._do_monotonic_check()

return self.monotonic_inc == 1
return self.monotonic_inc == 1

property is_monotonic_decreasing:
@property
def is_monotonic_decreasing(self):
if self.need_monotonic_check:
self._do_monotonic_check()

def __get__(self):
if self.need_monotonic_check:
self._do_monotonic_check()

return self.monotonic_dec == 1
return self.monotonic_dec == 1

cdef inline _do_monotonic_check(self):
cdef object is_unique
Expand Down Expand Up @@ -279,10 +276,9 @@ cdef class IndexEngine:
cdef _check_type(self, object val):
hash(val)

property is_mapping_populated:

def __get__(self):
return self.mapping is not None
@property
def is_mapping_populated(self):
return self.mapping is not None

cdef inline _ensure_mapping_populated(self):
# this populates the mapping
Expand Down
6 changes: 3 additions & 3 deletions pandas/_libs/tslibs/conversion.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,9 +203,9 @@ cdef class _TSObject:
# int64_t value # numpy dt64
# object tzinfo

property value:
def __get__(self):
return self.value
@property
def value(self):
return self.value
Copy link
Member

Choose a reason for hiding this comment

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

I've been kinda curious how this works namespace-wise. Why isn't this a recursion error?

Copy link
Member Author

@jschendel jschendel Dec 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, now that you mention it, this does seem a bit strange, even the way it was originally written with the legacy syntax. I tried creating similar classes in plain Python and they all raised.

That aside, looking further into the code, it doesn't seem like value be a property? Or should at least have a setter? Maybe I'm missing something obvious (not super familiar with this part of the codebase or Cython in general), but it looks like there are places where value is being set, e.g.

pandas/pandas/_libs/tslibs/conversion.pyx

Lines 250 to 256 in 0e16818

obj = _TSObject()
if is_string_object(ts):
return convert_str_to_tsobject(ts, tz, unit, dayfirst, yearfirst)
if ts is None or ts is NaT:
obj.value = NPY_NAT

(and many other examples further below that)

Trying something similar on master raises, albeit in plain Python:

In [1]: from pandas._libs.tslibs.conversion import _TSObject

In [2]: obj = _TSObject()

In [3]: obj.value = 10
---------------------------------------------------------------------------
AttributeError: attribute 'value' of 'pandas._libs.tslibs.conversion._TSObject' objects is not writable

So I'm a little confused as to how this working in the snippet I posted from the codebase, but again, could just be my relative inexperience with Cython speaking.

Copy link
Member

Choose a reason for hiding this comment

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

Are the test failures related to the change in this discussion?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gfyoung : I believe so, but I'm not 100% sure.

The test failure reads:

  File "pandas/_libs/tslibs/conversion.pyx", line 268, in pandas._libs.tslibs.conversion.convert_to_tsobject (pandas/_libs/tslibs/conversion.c:3771)
    obj.value = ts
AttributeError: can't set attribute

which is just a few lines below the snipped from conversion.pyx I posted.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, maybe it needs a setter as well. I though this was a problem as was trying to access a c-attribute from a non-cdef, but that's not the case.

Copy link
Member

@gfyoung gfyoung Dec 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Why don't you revert your changes to this property syntax so that you have a clean commit that passes all tests. Then you can mess around with this problematic instance in a separate commit if need be.

Copy link
Contributor

Choose a reason for hiding this comment

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

what version of cython supports this?
not averse to bumping it

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like 0.24.0. Was able to reproduce the error locally using 0.23.x, then get it working on 0.24.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok would take this as an update then to 0.24
need to update setup.py, some ci files, install.rst and add a more in whatsnew

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. Will open an issue to have something to reference in the whatsnew, and give this more visibility on the off chance that someone has an objection to the bump. And here I was thinking that this PR would be an easy and straightforward way to start getting into to the Cython part of the codebase!



cpdef int64_t pydt_to_i8(object pydt) except? -1:
Expand Down