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

jschendel · 2017-12-02T19:48:38Z

passes git diff upstream/master -u -- "*.py" | flake8 --diff

See the Cython docs for a comparison of the decorator syntax and legacy syntax. I only found a few instances of the legacy syntax; looks like most of the Cython code is already using decorator syntax.

codecov · 2017-12-02T21:40:39Z

Codecov Report

Merging #18602 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18602      +/-   ##
==========================================
- Coverage   91.46%   91.45%   -0.02%     
==========================================
  Files         157      157              
  Lines       51439    51439              
==========================================
- Hits        47051    47042       -9     
- Misses       4388     4397       +9

Flag	Coverage Δ
#multiple	`89.32% <ø> (ø)`	⬆️
#single	`40.6% <ø> (-0.11%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.81% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e16818...501e9ce. Read the comment docs.

codecov · 2017-12-02T21:40:40Z

Codecov Report

Merging #18602 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18602      +/-   ##
==========================================
- Coverage   91.59%   91.58%   -0.02%     
==========================================
  Files         155      155              
  Lines       51255    51255              
==========================================
- Hits        46948    46941       -7     
- Misses       4307     4314       +7

Flag	Coverage Δ
#multiple	`89.44% <ø> (ø)`	⬆️
#single	`40.67% <ø> (-0.11%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.81% <0%> (-0.1%)`	⬇️
pandas/util/testing.py	`82.01% <0%> (+0.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a764663...6b136cf. Read the comment docs.

jreback · 2017-12-02T22:09:19Z

pandas/_libs/tslibs/conversion.pyx

+    def value(self):
+        return self.value
+
+    @value.setter


don’t add this

Yeah, some 2.7 builds failed earlier without it, and it looked like there were places where value was being set, so I added that to see if it would resolve the issue since I couldn't reproduce locally. Looks like that wasn't the issue.

jbrockmendel · 2017-12-02T23:38:32Z

pandas/_libs/tslibs/conversion.pyx

-            return self.value
+    @property
+    def value(self):
+        return self.value


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

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.

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

@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.

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.

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.

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

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

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

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!

jreback · 2017-12-03T15:57:40Z

so the .values value is actually set only via a c call, IOW this is not a python settable propery

jbrockmendel · 2017-12-03T17:24:22Z

so the .values value is actually set only via a c call, IOW this is not a python settable propery

This 100% makes sense. Regardless, the property is almost certainly not needed.

jschendel · 2017-12-03T22:08:46Z

Would it be worthwhile to add a check for legacy syntax to lint.sh? Or is this something that's infrequent enough to not bother with?

jreback · 2017-12-03T23:11:11Z

i don’t think necessary to add a lint check here

jschendel · 2017-12-05T01:23:27Z

Green after rebase to include the Cython version bump. Didn't need to add a setter or any additional code.

jreback · 2017-12-06T11:27:38Z

thanks!

jschendel force-pushed the cython-property-syntax branch from 05a7d26 to 501e9ce Compare December 2, 2017 21:40

jreback requested changes Dec 2, 2017

View reviewed changes

jschendel force-pushed the cython-property-syntax branch from 501e9ce to 5a9f664 Compare December 2, 2017 22:23

jbrockmendel reviewed Dec 2, 2017

View reviewed changes

gfyoung added the Code Style Code style, linting, code_checks label Dec 3, 2017

jreback added the Build Library building on various platforms label Dec 3, 2017

jschendel mentioned this pull request Dec 3, 2017

BLD: Bump Cython version from 0.23 to 0.24 #18613

Closed

STYLE: Use property decorator syntax in Cython

6b136cf

jschendel force-pushed the cython-property-syntax branch from 5a9f664 to 6b136cf Compare December 4, 2017 20:21

jreback approved these changes Dec 6, 2017

View reviewed changes

jreback added this to the 0.22.0 milestone Dec 6, 2017

jreback merged commit 19ce05e into pandas-dev:master Dec 6, 2017

jschendel deleted the cython-property-syntax branch December 6, 2017 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jschendel commented Dec 2, 2017

codecov bot commented Dec 2, 2017

codecov bot commented Dec 2, 2017 •

edited

Loading

jreback Dec 2, 2017

jschendel Dec 2, 2017

jbrockmendel Dec 2, 2017

jschendel Dec 3, 2017 •

edited

Loading

gfyoung Dec 3, 2017

jschendel Dec 3, 2017

jreback Dec 3, 2017

gfyoung Dec 3, 2017 •

edited

Loading

jreback Dec 3, 2017

jschendel Dec 3, 2017

jreback Dec 3, 2017

jschendel Dec 3, 2017

jreback commented Dec 3, 2017

jbrockmendel commented Dec 3, 2017

jschendel commented Dec 3, 2017

jreback commented Dec 3, 2017

jschendel commented Dec 5, 2017

jreback commented Dec 6, 2017

	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

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

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

Conversation

jschendel commented Dec 2, 2017

codecov bot commented Dec 2, 2017

Codecov Report

codecov bot commented Dec 2, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschendel Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 3, 2017

jbrockmendel commented Dec 3, 2017

jschendel commented Dec 3, 2017

jreback commented Dec 3, 2017

jschendel commented Dec 5, 2017

jreback commented Dec 6, 2017

codecov bot commented Dec 2, 2017 •

edited

Loading

jschendel Dec 3, 2017 •

edited

Loading

gfyoung Dec 3, 2017 •

edited

Loading