[PROTO] Adding Cython framework for lite-functions (and more) #1416

rocco8773 · 2022-02-03T01:16:16Z

This is currently a prototype to see how we can incorporate Cython into plasmapy. I'm particularly curious in how...

To write cython code. (Using lite-functions as the prototype)
How to package and distribute with cython code.
How to develop with cython code.

Using cython in plasmapy means developers/contributors have an extra step to get off the ground, because, like c, cython code needs to be compiled. Cython looks very similar to Python and can be written in either .py files or .pyx files. .pyx files are specifically Cython files. Once this is done, the code needs to be cythonized (i.e. built/compile). In this process c code is generated from the source code and then the c code is compiled into share object files. Python can then import directly from these shared object files. However, these shared object files are platform specific and this is why we can not package with pre-compiled code. Luckily, these shared object files are automatically generate with pip installs, so there shouldn't be any extra steps for end users. For developers/contributors, we don't typically pip install so we have manually create the shared object files. This is done by...

Navigating to the root directory for plasmapy.
Executing the following command
python setup.py build_ext --inplace

This will create the shared object files in your repository. However, these are not under version control (i.e. excluded by .gitignore), so whenever cython code is modified or you switch to a new branch you'll likely need to re-compile.

To assist in cleaning up the compiled code I did modify the setup.py so that following commands will remove files generated by python setup.py build_ext --inplace.

python setup.py clean
python setup.py clean --all

If you want to see what file are deleted before cleaning, then you can use...

python setup.py clean --dry-run

codecov · 2022-02-03T01:41:53Z

Codecov Report

Merging #1416 (e343360) into cython (a624976) will not change coverage.
The diff coverage is n/a.

❗ Current head e343360 differs from pull request most recent head 0fb4c75. Consider uploading reports for the commit 0fb4c75 to get more accurate results

@@           Coverage Diff           @@
##           cython    #1416   +/-   ##
=======================================
  Coverage   97.15%   97.15%           
=======================================
  Files          77       77           
  Lines        7406     7406           
=======================================
  Hits         7195     7195           
  Misses        211      211

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 a624976...0fb4c75. Read the comment docs.

…p (isort will complain about this)

namurphy

Looking good! My suggested changes are all minor, and you're free to ignore them if you so choose.

The main thing I'm still wondering about, which I think we talked about yesterday, is on whether or not it specifically needs to be a numpy array as input, or if it could be a real or int. I think I remember you saying that yes, it does need to be a numpy array for the input.

If you're looking to eventually merge this prototype, we'd want to include the additional required steps in the contributor guide section on installing PlasmaPy for development.

Thank you for doing this!

namurphy · 2022-02-03T18:50:06Z

plasmapy/dispersion/_dispersionfunction.pyx

+    Z = 1j * np.sqrt(np.pi) * wofz(zeta)
+
+    return Z


Suggested change

Z = 1j * np.sqrt(np.pi) * wofz(zeta)

return Z

return 1j * np.sqrt(np.pi) * wofz(zeta)

[optional] This is a sort of refactoring I've often had pop up lately. Up to you.

setup.cfg

namurphy · 2022-02-03T19:02:07Z

setup.py

+        from importlib.machinery import EXTENSION_SUFFIXES
+
+        ext_suffixes = EXTENSION_SUFFIXES.copy()
+        ext_suffixes.append(".c")
+
+        cy_files = get_cython_files()
+        for file in cy_files:
+            for ext in ext_suffixes:
+                ext_file = file.with_suffix(ext)
+                if not ext_file.exists():
+                    continue
+
+                print(f"removing '{ext_file}'")
+
+                if "--dry-run" in sys.argv:
+                    continue
+
+                ext_file.unlink()  # delete extension file


[optional] My inclination would be to put this into its own function that gets called here, so as to not have implementation details in the if __name__ == "main" part. This goes along with the idea of having a main() function that gets called in this section of code. In this case there isn't much else here, so I'd have no problem leaving it as is. Up to you.

namurphy · 2022-02-03T19:05:00Z

setup.py

+
+def get_cython_files():
+    """Find all cython files (*.pyx) in plasmapy."""
+    return list(Path().glob("plasmapy/**/*.pyx"))


I hadn't seen the ** before, so I had to look up on the World Wide Web™ to learn that it mean multiple directories here. Glad to learn new Python!

namurphy · 2022-02-03T19:07:51Z

plasmapy/dispersion/_dispersionfunction.pyx

+# def plasma_dispersion_func_deriv(
+#     zeta: Union[complex, int, float, np.ndarray, u.Quantity]
+# ) -> Union[complex, float, np.ndarray, u.Quantity]:


Suggested change

# def plasma_dispersion_func_deriv(

# zeta: Union[complex, int, float, np.ndarray, u.Quantity]

# ) -> Union[complex, float, np.ndarray, u.Quantity]:

namurphy · 2022-02-03T20:03:11Z

setup.py

+    # by:
+    #     python setup.py build_ext --inplace
+    #
+    # this get executed with the command


Suggested change

# this get executed with the command

# this gets executed with the command

StanczakDominik

LGTM, one potential idea to make this introduce less code!

StanczakDominik · 2022-02-10T07:12:48Z

setup.py

+    # this get executed with the command
+    #       python setup.py clean
+    #
+    if "clean" in sys.argv:


clean is already a setup.py command and I would be wary of reimplementing it. Have you tried playing around with https://stackoverflow.com/questions/30359216/compile-a-cython-project-and-clean and see whether the default python setup.py clean --all does the trick?

People are generally moving away from setup.py in the python space in general... but that's probably an issue for another day.

I'm not overriding python setup.py clean, this still runs as expected. I'm adding an additional setup after the clean command is executed. The clean command does not know about, and thus can not remove, --inplace shared object files. All this is doing is seeing if the clean command is specified, letting the normal clean run, and then our setup.py removes the --inplace files.

Oooooh okay then, gotcha. 👍🏻

StanczakDominik · 2022-02-10T07:13:02Z

.github/workflows/testing.yml

+        pip install --progress-bar off twine
+        pip install --progress-bar off -r requirements/build.txt


good cleanup! 👍🏻

… to annotate cycthonize during build

github-actions · 2023-05-14T01:56:20Z

This pull request will be closed in 14 days due to a year of inactivity unless the stale label or comment is removed.

rocco8773 added 4 commits February 2, 2022 16:49

modify setup.py for cythonize-ing

7359ffe

add Cython to the build dependencies

c6b8eb4

updated comment to include shared objects

993d8a4

prototype cython file for dispersionfunction.py

6e40b80

rocco8773 requested review from namurphy and StanczakDominik February 3, 2022 01:16

add numpy as a build dependency

e72be31

rocco8773 added 7 commits February 2, 2022 17:47

add end of file blank line

c09e7be

make comment that cythonize import must come after the import of setu…

d2fb334

…p (isort will complain about this)

tell isort to ignore setup.py

bd8871d

implement more focused skipping of import linter-ing of setup.py

b3bfb77

appease grumpy black

846884c

add build requirements to .readthedocs.yml

8bbb7c2

try to leverage requirements files for the Packaging GA

8faf9c4

github-actions bot added the CI Related to continuous integration label Feb 3, 2022

learn how to spell requirements

e343360

namurphy reviewed Feb 3, 2022

View reviewed changes

rocco8773 changed the base branch from main to cython February 8, 2022 19:59

StanczakDominik approved these changes Feb 10, 2022

View reviewed changes

rocco8773 marked this pull request as draft February 10, 2022 19:05

rocco8773 added 3 commits February 10, 2022 12:14

add a simple docstring to test doc build

ca01ab9

do an import of the cython lite function

821957e

break out cleaning of --inplace files into separate func, add ability…

0fb4c75

… to annotate cycthonize during build

github-actions bot added the Stale Dormant issues & PRs which will be automatically closed if the label is not removed. label May 14, 2023

namurphy added the status: dormant PRs that are stalled label May 26, 2023

github-actions bot removed the Stale Dormant issues & PRs which will be automatically closed if the label is not removed. label Sep 9, 2023

namurphy added this to the Future milestone Apr 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PROTO] Adding Cython framework for lite-functions (and more) #1416

[PROTO] Adding Cython framework for lite-functions (and more) #1416

rocco8773 commented Feb 3, 2022

codecov bot commented Feb 3, 2022 •

edited

Loading

namurphy left a comment

namurphy Feb 3, 2022

namurphy Feb 3, 2022

namurphy Feb 3, 2022

namurphy Feb 3, 2022

namurphy Feb 3, 2022

StanczakDominik left a comment

StanczakDominik Feb 10, 2022

rocco8773 Feb 10, 2022

StanczakDominik Feb 11, 2022

StanczakDominik Feb 10, 2022

github-actions bot commented May 14, 2023

	# def plasma_dispersion_func_deriv(
	# zeta: Union[complex, int, float, np.ndarray, u.Quantity]
	# ) -> Union[complex, float, np.ndarray, u.Quantity]:

	# this get executed with the command
	# this gets executed with the command

		pip install --progress-bar off twine
		pip install --progress-bar off -r requirements/build.txt

[PROTO] Adding Cython framework for lite-functions (and more) #1416

Are you sure you want to change the base?

[PROTO] Adding Cython framework for lite-functions (and more) #1416

Conversation

rocco8773 commented Feb 3, 2022

codecov bot commented Feb 3, 2022 • edited Loading

Codecov Report

namurphy left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StanczakDominik left a 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

Choose a reason for hiding this comment

github-actions bot commented May 14, 2023

codecov bot commented Feb 3, 2022 •

edited

Loading