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.

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

DOC: Add Cython style guideline for DIPY. #1714

Merged
merged 1 commit into from Mar 8, 2019

Conversation

@jhlegarreta
Copy link
Contributor

commented Jan 4, 2019

Add Cython style guideline for DIPY.

Fixes #583.

@jhlegarreta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2019

A very first draft.

Somme comments:

Please have your say about them: either that we should incorporate them to the guideline as they are, or that we want to add them with specific changes.

One controversial point may be that of the Pointers & References: the Good syntax is not what C-people would recommend. However, since pointers (and references I guess) are to dwell on a line of their own, I guess it is fair to keep that.

Use of npy_intp for anything holding an index or shape value, when to use the ndarray Cython
syntax compared to the memoryviews compared to simple C arrays, how to use nogil to root out 
Python-side stuff and optimize speed, that kind of thing.

since my experience writing/reading Cython code is limited. So suggestions for content are welcome.

  • I ignore where the rules or compilation warnings like the one addressed in #1706 stem from, or where we could find the standard so that we can reference it and save listing each and every warning we find in the CI's. I've looked for a while and did not find any related documentation. If anyone happens to know that, please point to the URL.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCythonStyleGuide branch from 8eea49e to 118c286 Jan 4, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jan 4, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@def59ac). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1714   +/-   ##
========================================
  Coverage          ?   84.3%           
========================================
  Files             ?     115           
  Lines             ?   13790           
  Branches          ?    2186           
========================================
  Hits              ?   11625           
  Misses            ?    1656           
  Partials          ?     509
@matthew-brett

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

Are you proposing to add the style guidelines you pointed to. They seem reasonable.

@matthew-brett
Copy link
Member

left a comment

A couple of small comments.

doc/devel/coding_style_guideline.rst Outdated Show resolved Hide resolved
doc/devel/coding_style_guideline.rst Show resolved Hide resolved
doc/devel/coding_style_guideline.rst Outdated Show resolved Hide resolved
doc/devel/coding_style_guideline.rst Show resolved Hide resolved

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCythonStyleGuide branch from 118c286 to 4c13254 Jan 27, 2019

@jhlegarreta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

Thanks for the suggestions @matthew-brett !

@jhlegarreta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

Anyone having the bandwith to comment on the open questions? Thanks.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCythonStyleGuide branch from 4c13254 to 4f6e11c Feb 2, 2019

@skoudoro

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

Hi @jhlegarreta,

Can you rebase your PR ?

Please have your say about them: either that we should incorporate

I think you can incorporate what you pointed too.

I have not added guidelines related to what....

I will try to create a PR on you repo for this point

I ignore where the rules or compilation warnings like the one addressed in...

I am ok with that point.

@arokem, @Garyfallidis, any thought?

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCythonStyleGuide branch from 4f6e11c to a27b8ac Feb 7, 2019

@jhlegarreta
Copy link
Contributor Author

left a comment

a27b8ac rebased on master and added a few more sections to the guideline based on the link I found.

Some comments inline.

I may find to modify the examples using DIPY's Cython specific code, but the guidelines are already there.

Suggestions, contributions are welcome.

When declaring an error return value with the `except` keyword, use one space on
both sides of the `except`. If in a function definition, there should be no
spaces between the error return value and the colon `:`. Avoid `except *`
unless it is needed for functions returning `void`::

This comment has been minimized.

Copy link
@jhlegarreta

jhlegarreta Feb 7, 2019

Author Contributor

There are a few places where

Avoid `except *` unless it is needed for functions returning `void`:

sentence is not fulfilled in DIPY, e.g. in featurespeed.pyx:53:

cdef void c_extract(Feature self, Data2D datum, Data2D out) nogil except *:

This comment has been minimized.

Copy link
@skoudoro

skoudoro Feb 8, 2019

Member

I need to look deeper into this rule...


# Good
<float> i
<void *> s

This comment has been minimized.

Copy link
@jhlegarreta

jhlegarreta Feb 7, 2019

Author Contributor

This may be subject to some discussion/is may be a matter of tastes. I guess the line cythonutils.pyx:141

    memview._data = <char*> calloc(buffer_size, sizeof(float))

was may be not written following any specific guideline.

This comment has been minimized.

Copy link
@skoudoro

skoudoro Feb 8, 2019

Member

I am ok, with this rule, we can update our code on a new PR. Any other thought?

This comment has been minimized.

Copy link
@jhlegarreta

jhlegarreta Feb 8, 2019

Author Contributor

No. I'm OK with it. Yes, we should eventually converge to abide by these rules in our Cython code 📘 .


# Good
cdef int& i
cdef char * s

This comment has been minimized.

Copy link
@jhlegarreta

jhlegarreta Feb 7, 2019

Author Contributor

As said earlier in the PR, personally, I find clearer

cdef int &i

or

cdef char *s

but I am aware this may be controversial.

This comment has been minimized.

Copy link
@skoudoro

skoudoro Feb 8, 2019

Member

Personnally, I prefer just this:

cdef int& i
cdef char* s

I find it clearer because it avoid to mix different type on the same line. I see often this

cdef char *s, k, *p, t

which is confusing (to see 2 declaration types on the same line)

This comment has been minimized.

Copy link
@jhlegarreta

jhlegarreta Feb 8, 2019

Author Contributor

The example you pointed would not be allowed according to the Variable declaration section rules, since pointer variables should dwell on a line of their own 😅 .

Whether the dereference operator goes beside the type or not, as said, is subject to debate. I'd be OK with whatever is decided, would update the PR with it, and would adapt to it in DIPY.

I find that whatever is decided, the Casting rule/example should also abide by it: i.e. <void *> s or <void*> s.

doc/devel/coding_style_guideline.rst Outdated Show resolved Hide resolved
doc/devel/coding_style_guideline.rst Outdated Show resolved Hide resolved

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCythonStyleGuide branch from a27b8ac to d19163f Feb 8, 2019

@jhlegarreta

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

d19163f uses examples directly taken from the DIPY Cython code.

@skoudoro
Copy link
Member

left a comment

Thanks for the update @jhlegarreta, here some quick comment

doc/devel/coding_style_guideline.rst Outdated Show resolved Hide resolved
doc/devel/coding_style_guideline.rst Outdated Show resolved Hide resolved

# Good
<float> i
<void *> s

This comment has been minimized.

Copy link
@skoudoro

skoudoro Feb 8, 2019

Member

I am ok, with this rule, we can update our code on a new PR. Any other thought?


# Good
cdef int& i
cdef char * s

This comment has been minimized.

Copy link
@skoudoro

skoudoro Feb 8, 2019

Member

Personnally, I prefer just this:

cdef int& i
cdef char* s

I find it clearer because it avoid to mix different type on the same line. I see often this

cdef char *s, k, *p, t

which is confusing (to see 2 declaration types on the same line)

When declaring an error return value with the `except` keyword, use one space on
both sides of the `except`. If in a function definition, there should be no
spaces between the error return value and the colon `:`. Avoid `except *`
unless it is needed for functions returning `void`::

This comment has been minimized.

Copy link
@skoudoro

skoudoro Feb 8, 2019

Member

I need to look deeper into this rule...

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCythonStyleGuide branch 2 times, most recently from ff65019 to 2166e44 Feb 8, 2019

@skoudoro skoudoro referenced this pull request Mar 5, 2019
14 of 15 tasks complete
DOC: Add Cython style guideline for DIPY.
Add Cython style guideline for DIPY.

Fixes #583.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCythonStyleGuide branch from 2166e44 to 176a3e7 Mar 5, 2019

@jhlegarreta

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Travis CI failures are unrelated:

dipy/workflows/tests/test_reconst_mapmri.py
dipy/workflows/tests/test_align.py

are failing for different reasons unrelated to this topic. I ignore whether these have already been detected.

@skoudoro

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Thanks for the rebase @jhlegarreta.

@skoudoro

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

This is a good first version so I will go ahead and merge it. I will keep #583 open because this guideline needs a bit more work but it can be done on a new PR.

Thank @jhlegarreta for introducing this guideline.

@skoudoro skoudoro merged commit a3e598f into nipy:master Mar 8, 2019

4 checks passed

codecov/patch Coverage not affected.
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.