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: Stateful tractogram examples #1925

Merged
merged 12 commits into from Jul 29, 2019

Conversation

@frheault
Copy link
Contributor

commented Jul 26, 2019

After the new merge of PR #1812, most examples were not running.

Considering that before most trk saved and loaded by the tutorials were not valid, this PR is not only updating the function call, but also fixing the resulting TRK files involved.

@pep8speaks

This comment has been minimized.

Copy link

commented Jul 26, 2019

Hello @frheault, Thank you for updating !

Line 116:33: E128 continuation line under-indented for visual indent
Line 330:37: E128 continuation line under-indented for visual indent

Line 115:75: W291 trailing whitespace

Line 227:1: E402 module level import not at top of file

Line 59:1: E402 module level import not at top of file
Line 201:1: E402 module level import not at top of file

Line 33:1: E402 module level import not at top of file

Line 26:1: E402 module level import not at top of file
Line 69:1: E402 module level import not at top of file

Comment last updated at 2019-07-29 15:29:35 UTC
@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

Thank you for this @frheault! I will try them all because currently, the CI does not run the examples.

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

Coool! Thanks @frheault !

@frheault

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

I know it is a lot of files, but since it is almost exactly the same lines changed in every examples/workflows I thought that it would be faster for all of us.

Any comments that apply to all, I will fix them similarly everywhere.
Also now I tested them and open the files in MI-Brain & Trackvis and they are valid !
image

@arokem
Copy link
Member

left a comment

Looks great! A couple of small things.

doc/examples/afq_tract_profiles.py Outdated Show resolved Hide resolved
doc/examples/afq_tract_profiles.py Outdated Show resolved Hide resolved
@@ -15,14 +15,21 @@
"""

# Enables/disables interactive visualization

This comment has been minimized.

Copy link
@arokem

arokem Jul 26, 2019

Member

This comment should now move down, to be adjacent to the line of code it refers to.

@codecov-io

This comment has been minimized.

Copy link

commented Jul 27, 2019

Codecov Report

Merging #1925 into master will increase coverage by 0.73%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1925      +/-   ##
==========================================
+ Coverage   83.89%   84.62%   +0.73%     
==========================================
  Files         118      118              
  Lines       14679    14682       +3     
  Branches     2326     2326              
==========================================
+ Hits        12315    12425     +110     
+ Misses       1814     1714     -100     
+ Partials      550      543       -7
Impacted Files Coverage Δ
dipy/workflows/tracking.py 96.55% <100%> (+0.12%) ⬆️
dipy/viz/app.py 52.34% <0%> (+0.39%) ⬆️
dipy/io/vtk.py 56.31% <0%> (+0.97%) ⬆️
dipy/viz/panel.py 83.09% <0%> (+1.4%) ⬆️
dipy/io/utils.py 69.35% <0%> (+2.41%) ⬆️
dipy/testing/__init__.py 78.12% <0%> (+3.12%) ⬆️
dipy/viz/regtools.py 36.79% <0%> (+4.79%) ⬆️
dipy/utils/optpkg.py 78.26% <0%> (+8.69%) ⬆️
dipy/viz/__init__.py 90% <0%> (+10%) ⬆️
dipy/workflows/stats.py 92% <0%> (+30.4%) ⬆️
... and 1 more
frheault and others added 4 commits Jul 27, 2019
Update doc/examples/afq_tract_profiles.py
Co-Authored-By: Ariel Rokem <arokem@gmail.com>
Update doc/examples/afq_tract_profiles.py
Co-Authored-By: Ariel Rokem <arokem@gmail.com>
@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

When I generate the documentation, I got the following error with streamline_tools.py tutorial:

*************************************************************
streamline_tools.py
*************************************************************
Dataset is already in place. If you want to fetch it again please first remove the folder C:\Users\skoudoro\.dipy\stanford_hardi
Dataset is already in place. If you want to fetch it again please first remove the folder C:\Users\skoudoro\.dipy\stanford_hardi
Dataset is already in place. If you want to fetch it again please first remove the folder C:\Users\skoudoro\.dipy\stanford_hardi
Dataset is already in place. If you want to fetch it again please first remove the folder C:\Users\skoudoro\.dipy\stanford_hardi
ERROR:root:Voxel space values higher than dimensions
Traceback (most recent call last):
  File "..\\..\\tools\\make_examples.py", line 166, in <module>
    run_script()
  File "..\\..\\tools\\make_examples.py", line 148, in run_script
    exec(f.read(), namespace)
  File "<string>", line 238, in <module>
  File "c:\users\skoudoro\devel\dipy\dipy\io\streamline.py", line 251, in save_trk
    save_tractogram(sft, filename, bbox_valid_check=bbox_valid_check)
  File "c:\users\skoudoro\devel\dipy\dipy\io\streamline.py", line 39, in save_tractogram
    raise ValueError('Bounding box is not valid in voxel space, cannot ' +
ValueError: Bounding box is not valid in voxel space, cannot save a valid file if some coordinates are invalid
@frheault

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

@skoudoro I just fixed it. I guess it me that forgot to save or hit CTRL-Z by accident. The input space was VOX, not RASMM. Perfect catch using the value error, I guess my own code save me !

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

I guess my own code save me!

Nice, Love it! I will try again to generate the doc, check the result and if Travis is Happy, will go ahead and merge it.

Does anyone want to have a look?

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

bundle_extraction.py example stopped to work. I do not think it is related to your PR but can you try to run it quickly and let me know. (Maybe it is my version of fury). Thank you

@frheault

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

@skoudoro works fine on my side. I just resetup recently, nothing special that I remember (with vtk/fury)

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Thank you @frheault, merging

@skoudoro skoudoro merged commit b9d0736 into nipy:master Jul 29, 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
frheault added a commit to frheault/dipy that referenced this pull request Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.