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.

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

FIX: Skip refcheck in ArraySequence construction/extension #719

Merged
merged 1 commit into from
Jan 25, 2019

Conversation

arokem
Copy link
Member

@arokem arokem commented Jan 23, 2019

I stumbled into this in the course of my work on pyAFQ.

For testing, part of the code reads data from a trk file. This seems to cause some issues in this method, but only on 3.7: https://travis-ci.org/arokem/pyAFQ/jobs/483151878. Unfortunately, I was not able to reproduce these errors locally on my machine...

After reading about what this might be, and implementing the proposed change, these tests are now passing on the Travis build for that branch, though.

Any objections to this?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.812% when pulling 872ce94 on arokem:array_sequence_resize into f9e7670 on nipy:master.

@codecov-io
Copy link

Codecov Report

Merging #719 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #719   +/-   ##
======================================
  Coverage    89.1%   89.1%           
======================================
  Files          93      93           
  Lines       11468   11468           
  Branches     1991    1991           
======================================
  Hits        10218   10218           
  Misses        911     911           
  Partials      339     339
Impacted Files Coverage Δ
nibabel/streamlines/array_sequence.py 100% <100%> (ø) ⬆️

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 f9e7670...872ce94. Read the comment docs.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Okay, I've looked through this file, and tried to think through the ways that you might hit this function, and it seems like the ndarray object should be pretty isolated, so the refcount doesn't seem to be going up for a reason we need to guard against.

So I'm inclined to merge.

@matthew-brett @MarcCote This is in code you two co-wrote, so if you see a reason we need to seriously worry about breaking other objects sharing a buffer, then please make some noise.

I'll plan to merge on Friday.

@matthew-brett
Copy link
Member

I defer to @MarcCote on this one ...

@MarcCote
Copy link
Contributor

After looking at the resize documentation, I'd say let's merge this. It should be fine.

@effigies effigies changed the title Don't refcheck on this resize, to avoid errors FIX: Skip refcheck in ArraySequence construction/extension Jan 25, 2019
@effigies
Copy link
Member

Thanks for the review.

@effigies effigies merged commit ad6b890 into nipy:master Jan 25, 2019
@effigies effigies added this to the 2.4.0 milestone Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants