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

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
Copy link

pep8speaks 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
Copy link
Member

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

@Garyfallidis
Copy link
Contributor

Coool! Thanks @frheault !

@frheault
Copy link
Contributor Author

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

Copy link
Contributor

@arokem arokem left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@codecov-io
Copy link

codecov-io 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

@skoudoro
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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

@skoudoro
Copy link
Member

Thank you @frheault, merging

@skoudoro skoudoro merged commit b9d0736 into dipy:master Jul 29, 2019
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants