MAINT: fully cythonize `funcnet.CouplingAnalysis`

fkuehlein commented 9 months ago

resolves #187, see commit message for details

ntfrgl commented 9 months ago

Does this also resolve #162, as per your earlier comment?

fkuehlein commented 9 months ago

Right, creating the PR I noticed I hadn't looked into that yet, which is why it is only a draft now. Tests are passing, but apparently that didn't help before. Will check on it later today!

codecov[bot] commented 9 months ago

Codecov Report

Merging #195 (5ed5b10) into master (6dc3f4c) will increase coverage by 4.04%. Report is 185 commits behind head on master. The diff coverage is 69.60%.

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
+ Coverage   48.00%   52.04%   +4.04%     
==========================================
  Files          44       49       +5     
  Lines        6877     7473     +596     
==========================================
+ Hits         3301     3889     +588     
- Misses       3576     3584       +8

Files	Coverage Δ
src/pyunicorn/__init__.py	`100.00% <100.00%> (ø)`
src/pyunicorn/climate/__init__.py	`100.00% <100.00%> (ø)`
src/pyunicorn/climate/coupled_tsonis.py	`83.33% <ø> (ø)`
src/pyunicorn/climate/hilbert.py	`78.94% <ø> (ø)`
src/pyunicorn/climate/partial_correlation.py	`95.23% <ø> (ø)`
src/pyunicorn/climate/rainfall.py	`100.00% <100.00%> (ø)`
src/pyunicorn/climate/spearman.py	`100.00% <ø> (ø)`
src/pyunicorn/climate/tsonis.py	`94.33% <100.00%> (ø)`
src/pyunicorn/core/__init__.py	`100.00% <100.00%> (ø)`
src/pyunicorn/core/_ext/types.py	`100.00% <100.00%> (ø)`
... and 39 more

ntfrgl commented 9 months ago

Thank you!

Please review my minor changes, and please add a few more tests specifically for the Cython functions (not mediated by the library classes), in order to increase our confidence in the Cython port.

fkuehlein commented 9 months ago

Awesome, looks good so far! I had been wondering at some point if the flattening was still of any advantage in Cython, could have asked you in the first place I guess.. :)

Thank's for the style corrections, too. Had been looking for static code analysis tools for Cython but couldn't really find anything good at first sight. I now ran into cython-lint and used it to go through funcnet/_ext/numerics.pyx again in c206447. Might be worth extending such a thing to other Cython modules some time? Or are there other static code analysis tools for Cython that come to your mind?

I also introduced the type aliases on all remaining occasions in funcnet.CouplingAnalysis itself (and test_coupling_analyis.py) in f09b1d6 (and 5ed5b10...).

Concerning the direct testing of the Cython functions:

I'm not sure I understand what their use would be. All functions in funcnet._ext.numerics have what are basically just wrapper methods in funcnet.CouplingAnalysis, which we are already testing, right?

Except for funcnet.CouplingAnalysis.get_nearest_neighbors() indeed, which so far is only tested through test_mutual_information() and test_information_transfer(). Trying to implement a test for that method, I spent half the day looking through it. I did find that there are redundant loops and variables defined in several places (see my comments below) that make it a bit harder to read (the code appears to be adapted from a more generally applicable algorithm, but has lost its wider applicability anyway due to the adaptations). I mostly grasped its functionality by now, but still don't really understand the special role of the z dimension in it. Understanding it would be essential to create a sensible test fixture and an expected result to compare to.

Therefore, considering that the existing tests already formally cover the method and assuming that they actualIy do what they suggest to, I concluded it would be better to file a seperate issue for this and invest my efforts in other points on our agenda first. Would you agree?

Please review and let me know if I'm missing something!

ntfrgl commented 9 months ago

Yes, that's a great suggestion. You could add cython-lint to our test suite by:

adding it to the testing dependencies in setup.cfg,
adding a new tox test environment in setup.cfg which calls cython-lint,
and configuring cython-lint itself in pyproject.toml.

Otherwise, this is good to go. It's probably not worth spending more of your time on understanding these methods at this stage.

fkuehlein commented 9 months ago

Awesome! Thanks again for reviewing, I will hence merge this PR and file separate issues for the discussed points.

pik-copan / pyunicorn

MAINT: fully cythonize `funcnet.CouplingAnalysis` #195

Codecov Report