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

BF: added check to avoid infinite loop on consecutive coordinates. #1804

Merged
merged 1 commit into from Jul 30, 2019

Conversation

@jchoude
Copy link
Contributor

commented Apr 12, 2019

In the _streamline_in_mask function, if two or more consecutive points shared exactly the same coordinates, the loop would never finish. Added a quick check on the distance to avoid such case.

This is a test that is already present in our in house code and has been tested extensively.

@skoudoro skoudoro added this to the 1.0 milestone Apr 23, 2019

@jchoude

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@skoudoro Is there something I could do to make this go through? I would really like to have this in master (for my own needs ;) ). The CI failures are unrelated to what I changed and seem to be in the line of the Numpy / Cython issues.

I know I could merge it, but I don't think that's good style to merge my own changes without any other review (even for 2 lines, like here).

@skoudoro

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Hi @jchoude,

Do not worry about the CI error. But, Can you add a small test for this case? (see @MarcCote comment here during your issue #1805 )

And then, I will go ahead and merge it. Thank you

@jchoude

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Ok. Just FYI, this is not the fix for #1805, but is still related.

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Okay this looks good to me too. Small test all is needed.

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Hi @jchoude, Any chances to have a small test?

@jchoude

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Quick question @skoudoro : how can I test an infinite loop condition? If I just call my test on master, it will simply run forever. I'm not aware of facilities to test such a use case...

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Good question 😄! I propose you to just create a small wrapper of your function where you compute the time and, after a certain amount of time, you raise an Exception. In your test, check if this exception is raised or not. Tell me if I am not clear...

@jchoude

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

thanks @skoudoro for the suggestion.

However, I feel like this would probably add a lot of complexity to the testing framework. In order for this to work. I would need to spawn the method call in another thread, wait for the timer in the current thread, and after that kill the spawned thread.

It can be done, but don't you fell it makes the test a bit heavyweight?

@arokem

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

It can be done, but don't you fell it makes the test a bit heavyweight?

ok, you are right, just a bit heavyweight 😄

If we can't come up with a test for this (I can't), maybe lets go ahead with this.

Sounds good. Thank you for this PR @jchoude! merging!

@skoudoro skoudoro merged commit 2d63e7c into nipy:master Jul 30, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.