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

Jump to bottom

Cython style: prefer cnp.float32_t and cnp.float64_t for clarity #6303

Merged
merged 8 commits into from Apr 5, 2022
Merged

Cython style: prefer cnp.float32_t and cnp.float64_t for clarity #6303

merged 8 commits into from Apr 5, 2022

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Mar 27, 2022

Description

closes #3496

Replaces use of float types for 32-bit floating point variables with cnp.float32_t to avoid confusion with Python's float (which is 64-bit).

For consistency, also replace double with cnp.float64_t

Checklist

  • Docstrings for all functions
  • Gallery example in ./doc/examples (new features only)
  • Benchmark in ./benchmarks, if your changes aren't covered by an
    existing benchmark
  • Unit tests
  • Clean style in the spirit of PEP8
  • Descriptive commit messages (see below)

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.
  • There is a bot to help automate backporting a PR to an older branch. For
    example, to backport to v0.19.x after merging, add the following in a PR
    comment: @meeseeksdev backport to v0.19.x
  • To run benchmarks on a PR, add the run-benchmark label. To rerun, the label
    can be removed and then added again. The benchmark output can be checked in
    the "Actions" tab.

@grlee77
change cdef float types to cnp.float32_t
50bf34f 
replace double with cnp.float64_t
@grlee77 grlee77 added the 🔧 type: Maintenance Refactoring and maintenance of internals label Mar 27, 2022
@grlee77 grlee77 changed the title Cython style prefer cnp.float32_t and cnp.float64_t for clarity Cython style: prefer cnp.float32_t and cnp.float64_t for clarity Mar 27, 2022
rfezzani
rfezzani reviewed Mar 28, 2022
View reviewed changes
Copy link
Member

@rfezzani rfezzani left a comment

Choose a reason for hiding this comment

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

Thank you @grlee77! I proposed fixes for the few cnp imports that made the CI fail. I also suggest to cimport numpy as cnp instead of np in _marching_cubes_lewiner_cy.pyx for consistency. Otherwise everything looks good ;)

skimage/filters/rank/core_cy.pxd Outdated Show resolved Hide resolved
skimage/filters/rank/core_cy_3d.pxd Outdated Show resolved Hide resolved
skimage/filters/rank/core_cy_3d.pxd Outdated Show resolved Hide resolved
skimage/filters/rank/core_cy.pxd Outdated Show resolved Hide resolved
skimage/graph/heap.pxd Show resolved Hide resolved
skimage/measure/_marching_cubes_lewiner_cy.pyx Show resolved Hide resolved
skimage/measure/_marching_cubes_lewiner_cy.pyx Show resolved Hide resolved
skimage/measure/_marching_cubes_lewiner_cy.pyx Show resolved Hide resolved
skimage/measure/_marching_cubes_lewiner_cy.pyx Show resolved Hide resolved
skimage/measure/_marching_cubes_lewiner_cy.pyx Outdated Show resolved Hide resolved
grlee77 and others added 2 commits March 28, 2022 09:15
@grlee77
Fix missing numpy cimport statements
ddffc19 
Co-authored-by: Riadh <r.fezzani@vitadx.com>
@grlee77
fix missing import
c4513ad
@grlee77
Copy link
Contributor Author

grlee77 commented Mar 28, 2022

Thanks @rfezzani, I had missed uploading a second commit where I had fixed the missing imports. I have now incorporated the other suggestions here as well.

lagru
lagru reviewed Mar 28, 2022
View reviewed changes
Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

The proposed changes look good so far. However I think you missed at a few files:

  • float in _hoghistogram.pyx line 59
  • doubles in _texture.pyx, e.g. 12, 137, ...
  • a lot of doubles in _draw.pyx, e.g. 297, 298, ...
  • censure_cy.pyx
  • _find_contours_cy.pyx
  • ...

Are those intentional? I didn't know how to comment in those unchanged files directly, sorry. But I'm happy to push to this PR if you want.

skimage/filters/rank/core_cy.pxd Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Mar 28, 2022 •
edited

Hello @grlee77! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-03-29 07:22:56 UTC

@grlee77
Copy link
Contributor Author

grlee77 commented Mar 28, 2022

We can potentially follow up with a PR to exclude these style-based commits from the Git Blame view here:
https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

grlee77 and others added 2 commits March 28, 2022 17:30
@grlee77
pep8 fix
50577a6
@lagru
Convert a few left-over double to cnp.float64_t
2817f2d 
I wasn't sure whether to keep "double" for the extern function
definition e.g. in _unwrap_2d.pyx and _unwrap_3d.pyx but I opted for the
explicit and consistent option.
lagru
lagru reviewed Mar 29, 2022
View reviewed changes
skimage/restoration/_unwrap_2d.pyx
Comment on lines +11 to +12
cnp.float64_t *wrapped_image,
cnp.float64_t *UnwrappedImage,
Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure whether to keep "double" for the extern function
definition e.g. in _unwrap_2d.pyx and _unwrap_3d.pyx but I opted for the
explicit and consistent option.

@lagru
Copy link
Member

lagru commented Mar 29, 2022

@grlee77 I took the liberty of converting a few left-over doubles and pushing. I hope that was alright. :)

lagru
lagru approved these changes Mar 29, 2022
View reviewed changes
Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

Looks all good. Thanks @grlee77.

@alexdesiqueira alexdesiqueira merged commit 30a426c into scikit-image:main Apr 5, 2022
@alexdesiqueira
Copy link
Member

Thanks @grlee77 @rfezzani @lagru!

alexdesiqueira pushed a commit to alexdesiqueira/scikit-image that referenced this pull request May 24, 2022
@grlee77 @lagru
Cython style: prefer cnp.float32_t and cnp.float64_t for clarity (sci…
81be140 
…kit-image#6303)

* change cdef float types to cnp.float32_t

replace double with cnp.float64_t

* Fix missing numpy cimport statements

Co-authored-by: Riadh <r.fezzani@vitadx.com>

* fix missing import

* more double->cnp.float64_t

* np.double -> np.float64

* fix a couple remaining float->cnp.float32_t

* pep8 fix

* Convert a few left-over double to cnp.float64_t

I wasn't sure whether to keep "double" for the extern function
definition e.g. in _unwrap_2d.pyx and _unwrap_3d.pyx but I opted for the
explicit and consistent option.

Co-authored-by: Riadh <r.fezzani@vitadx.com>
Co-authored-by: Lars Grüter <lagru@mailbox.org>
@lagru lagru added this to the 0.20 milestone Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfezzani rfezzani rfezzani approved these changes

@lagru lagru lagru approved these changes

Assignees
No one assigned
Labels
🔧 type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Milestone
0.20
Development

Successfully merging this pull request may close these issues.

Cython guidelines: discouraging the word float
5 participants
@grlee77 @pep8speaks @lagru @alexdesiqueira @rfezzani