[Ctypes] TVB-2719 Use ctypes instead of cython #40

ayan-b · 2020-06-16T03:14:25Z

Closes #11 & closes #36

gdist.py

gdist_c_api.cpp

gdist.py

maedoc

Thanks for this first step towards making the gdist module easier to package and distribute. I would approve and let you choose which of the review points to follow up on, though some will be naturally obligatory, such as using the right DLL extension on different platforms.

maedoc · 2020-06-16T12:22:16Z

A last minor comment: I don't see why C++14 is needed, and it would be easier to not worry about MSVC if you were using C++11 or earlier.

ayan-b · 2020-06-16T12:28:02Z

@maedoc Thanks! The PR is still in WIP and I am working on it.

A last minor comment: I don't see why C++14 is needed, and it would be easier to not worry about MSVC if you were using C++11 or earlier.

I am using range-based for loop (which can easily be changed to standard for loop), so added c++14 to command line arguments.

codecov · 2020-06-22T14:10:01Z

Codecov Report

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

@@           Coverage Diff            @@
##             trunk      #40   +/-   ##
========================================
  Coverage         ?   81.96%           
========================================
  Files            ?        4           
  Lines            ?      122           
  Branches         ?        0           
========================================
  Hits             ?      100           
  Misses           ?       22           
  Partials         ?        0

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 106a7b8...120c1b5. Read the comment docs.

liadomide · 2020-06-29T12:50:07Z

These are cosmetics, but:

I suggest to have one unit-test comparing the currently computed geodesic matrix of distances with a matrix that was saved from the previous stable version of tvb-gdist (version 2.0.1 for example). Maybe use cortex_16384.zip in this test (then it will need to be committed in data folder).
Also, I see the root folder https://github.com/the-virtual-brain/tvb-gdist getting crowded. Could we maybe move some of the files inside geodesic_library ?

ayan-b · 2020-06-30T08:49:50Z

Not really sure why macOS and Windows jobs are failing sometimes with segmentation fault.

ayan-b · 2020-06-30T08:52:17Z

I suggest to have one unit-test comparing the currently computed geodesic matrix of distances with a matrix that was saved from the previous stable version of tvb-gdist (version 2.0.1 for example). Maybe use cortex_16384.zip in this test (then it will need to be committed in data folder).

cortex_16384 takes a lot of time, so I have tested with outer_skull_642, inner_skull_642 and scalp_1082.

Also, I see the root folder https://github.com/the-virtual-brain/tvb-gdist getting crowded. Could we maybe move some of the files inside geodesic_library ?

gdist.pyx, geodesic.py and alternative_geodesic.py can be removed and gdist_c_cpi.h, gdist_c_api.cpp can go inside geodesic_library.

maedoc · 2020-06-30T10:55:16Z

Lia's previous comment about comparing versions is very pertinent. I would also recommend some "synthetic" geometry like a circle and compare distances to known geodesic arc distances.

If you are feeling curious you could even implement something more interesting such as a surface with a known path integral e.g. https://math.stackexchange.com/questions/45089/what-is-the-length-of-a-sine-wave-from-0-to-2-pi

no obligation though really :)

liadomide · 2020-07-01T09:43:11Z

.travis.yml

-  - pip install pytest~=3.6.1
-  - pytest
+  - pip3 install nose2 nose2[coverage_plugin]>=0.6.5
+  - nose2 --with-coverage


I would have a preference to keep using pytest, for consistency with the other TVB builds.
Anyway, nose does not solve the seg fault issue.

I am not sure but it appears to be a pytest issue.

why you say it is pytest?
the build are still failing, and tests are not really execute in the latest Travis run.... am I missing something?

enthought/comtypes#204 I got this same error. The current failures are due to something else. I am still investigating and will report you back if nose actually fixes the issue.

ayan-b changed the title ~~TVB 2719 Use ctypes instead of cython~~ TVB-2719 Use ctypes instead of cython Jun 16, 2020

liadomide requested a review from maedoc June 16, 2020 12:03

liadomide assigned ayan-b Jun 16, 2020

liadomide added this to the GSOC2020 milestone Jun 16, 2020

ayan-b changed the title ~~TVB-2719 Use ctypes instead of cython~~ [WIP] TVB-2719 Use ctypes instead of cython Jun 16, 2020