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

BF: Avoid using memview in struct (Cython 0.28) #1439

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@MarcCote
Contributor

MarcCote commented Mar 1, 2018

Replace memview's object in Centroid struct with a memview pointer.
Added a unit test that uses a Python metric with QuickBundles clustering.
Fixed bench_streamlines.py script.

@skoudoro we can drop this PR in favor of #1434 since you already made the necessary modification (and it is much cleaner than what I have). However, you might want to use the fixed bench_streamlines.py script, the unit test test_quickbundles_with_python_metric in test_quickbundles.py and the fix in metricspeed.pyx

@MarcCote MarcCote requested a review from skoudoro Mar 1, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Mar 1, 2018

Codecov Report

Merging #1439 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1439      +/-   ##
==========================================
+ Coverage   87.38%   87.39%   +<.01%     
==========================================
  Files         237      237              
  Lines       30069    30090      +21     
  Branches     3232     3235       +3     
==========================================
+ Hits        26277    26298      +21     
  Misses       3043     3043              
  Partials      749      749
Impacted Files Coverage Δ
dipy/segment/tests/test_quickbundles.py 98.31% <100%> (+0.36%) ⬆️

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 95a9cca...85b06cd. Read the comment docs.

@skoudoro

Thanks for this change @MarcCote. I will integrate them in #1434.

Concerning metricspeed.pyx I wonder why we need this change. I think we cast it before calling c_dist, right?

for datum in rdata:
datum.setflags(write=False)
clusters = qb.cluster(rdata)

This comment has been minimized.

@skoudoro

skoudoro Mar 1, 2018

Member

unused, what is the goal here ?

This comment has been minimized.

@MarcCote

MarcCote Mar 1, 2018

Contributor

It was just to check the code can run without segfault even if data is read only.

This comment has been minimized.

@skoudoro

skoudoro Mar 1, 2018

Member

it makes sense now, thank you

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 1, 2018

Ok, I see why for metricspeed.pyx

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Mar 1, 2018

Yeah, we need to that trick in metricspeed.pyx (similar to what is done in clusters_centroid2clustermap_centroid) because the memview we created manually misses the reference to its Python counterpart (memview pointer in memview_slice struct).

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Mar 3, 2018

Close in favor of #1434

@MarcCote MarcCote closed this Mar 3, 2018

@MarcCote MarcCote deleted the MarcCote:fix_struct_memview branch Mar 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment