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

Modifications for building docs with python3 #1091

Merged
merged 4 commits into from Aug 2, 2016

Conversation

Projects
None yet
6 participants
@ghoshbishakh
Member

ghoshbishakh commented Jun 28, 2016

Change apigen.py for python3 support
And minor PEP8 changes
fixes #912

@ghoshbishakh ghoshbishakh changed the title from Modifications in apigen.py for building docs with python3 to Modifications for building docs with python3 Jun 28, 2016

@coveralls

This comment has been minimized.

coveralls commented Jun 28, 2016

Coverage Status

Coverage remained the same at 82.88% when pulling b15714b on ghoshbishakh:docsPy3 into 220d883 on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Jun 28, 2016

Current coverage is 80.84% (diff: 100%)

Merging #1091 into master will not change coverage

@@             master      #1091   diff @@
==========================================
  Files           200        200          
  Lines         23023      23023          
  Methods           0          0          
  Messages          0          0          
  Branches       2309       2309          
==========================================
  Hits          18614      18614          
  Misses         3933       3933          
  Partials        476        476          

Powered by Codecov. Last update 63f6a5f...37fc41e

@coveralls

This comment has been minimized.

coveralls commented Jun 28, 2016

Coverage Status

Coverage remained the same at 82.88% when pulling b15714b on ghoshbishakh:docsPy3 into 220d883 on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Jul 1, 2016

Nice! But documentation still does not build on Python 3 with this fix. Still need to also adjust this file to be Python 3 compliant: https://github.com/nipy/dipy/blob/master/doc/sphinxext/github.py

This line, for starters: https://github.com/nipy/dipy/blob/master/doc/sphinxext/github.py#L39

(that's where I get an error raised)

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 1, 2016

@arokem

This comment has been minimized.

Member

arokem commented Jul 1, 2016

I am not sure. Could it be that your system is getting the github sphinx extension from somewhere else?

@arokem

This comment has been minimized.

Member

arokem commented Jul 1, 2016

Either way, it's still not Python 3 compliant.

Try:

cd ~/source/dipy/doc/sphinxext
python -c"import github"
@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 1, 2016

@arokem yes I will fix that file. I have to check why I am not getting the error.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 1, 2016

@arokem sorry my bad. I was using python2 sphinx as default. I found that numpydoc extension also crashes. I think removing it from the sphinxext folder will be better as we can use the lastest numpydoc without shipping an older version. What do you recommend?

@arokem

This comment has been minimized.

Member

arokem commented Jul 1, 2016

You can also get python 3 compliant versions of all these here: https://github.com/uwescience/shablona/tree/master/doc/sphinxext

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 1, 2016

@arokem okey then I will include those.

@coveralls

This comment has been minimized.

coveralls commented Jul 1, 2016

Coverage Status

Coverage increased (+0.03%) to 82.908% when pulling 40790b7 on ghoshbishakh:docsPy3 into 220d883 on nipy:master.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 12, 2016

Looks good. Thanks @ghoshbishakh
Is there anything left on this PR? If not I will be happy to merge it.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 12, 2016

@Garyfallidis Not much is left. But I have not been able to test it completely with python3 as it takes a long time. You may merge it as I can make patches later also.
By completely I mean with all examples.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 12, 2016

Why does it take a long time?

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 12, 2016

@Garyfallidis Because I have only 4GB RAM and the build process requires about 16GB. And using swap it takes about 20 hours to build the entire doc.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 12, 2016

I think there are only a few examples that need more memory than usual. DKI and Life. You may want to disable them by commenting them at doc/examples/valid_examples.txt
Here is what I think may require more memory.
linear_fascicle_evaluation.py
reconst_dki.py
Let me know if that helped and if you can now compile the documentation faster.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 12, 2016

@Garyfallidis ok. I will build it and post a link here.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 13, 2016

I am getting this error:

segment_extending_clustering_framework.py
segment_clustering_features.py
Nb. clusters: 4
Cluster sizes: <map object at 0x7fa07c621588>
Nb. clusters: 4
Cluster sizes: <map object at 0x7fa07d5eb3c8>
Traceback (most recent call last):
  File "../tools/make_examples.py", line 122, in <module>
    exec(open(script).read(), namespace)
  File "<string>", line 172, in <module>
IndexError: too many indices for array
Makefile:122: recipe for target 'rstexamples-stamp' failed
make: *** [rstexamples-stamp] Error 1
make html  1442.06s user 18.58s system 134% cpu 18:07.87 total

screenshot from 2016-07-13 15-37-10

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 20, 2016

Hi @MarcCote have you seen this? Any ideas?

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 21, 2016

I ran the code for segment_clustering_features.py separately and I get this:

python segment_clustering_features.py 
Nb. clusters: 4
Cluster sizes: <map object at 0x7faf5bcc55f8>
Nb. clusters: 4
Cluster sizes: <map object at 0x7faf5df22358>
Traceback (most recent call last):
  File "segment_clustering_features.py", line 172, in <module>
    fvtk.add(ren, fvtk.point(centers[:, 0, :], colormap_full, point_radius=0.2))
IndexError: too many indices for array

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jul 22, 2016

@ghoshbishakh: @Garyfallidis just told me about your issue with segment_clustering_features.py. I just made a PR on your branch. That should resolve the issue. Basically, since in Python3 map(...) returns a generator, I had to cast them into list objects.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 23, 2016

What did just happen? I merged @MarcCote 's PR and pulled it locally and then made a push and this happened. 😟

@ghoshbishakh ghoshbishakh force-pushed the ghoshbishakh:docsPy3 branch from 0174213 to 40790b7 Jul 23, 2016

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 23, 2016

@MarcCote seems I have made a successful reset! Can you somehow make the PR again?

@coveralls

This comment has been minimized.

coveralls commented Jul 23, 2016

Coverage Status

Coverage remained the same at 82.908% when pulling 40790b7 on ghoshbishakh:docsPy3 into c09a194 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 23, 2016

Coverage Status

Coverage remained the same at 82.908% when pulling 40790b7 on ghoshbishakh:docsPy3 into c09a194 on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Jul 23, 2016

You might want to both be rebased on top of current master before trying
again :-)

On Sat, Jul 23, 2016 at 7:18 AM, Coveralls notifications@github.com wrote:

[image: Coverage Status] https://coveralls.io/builds/7131096

Coverage decreased (-0.4%) to 82.474% when pulling 40790b7
40790b7
on ghoshbishakh:docsPy3
into c09a194
c09a194
on nipy:master
.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1091 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHPNkVJg8_Or_nxifCSqeJD6Mh52b0cks5qYiK0gaJpZM4I_8U0
.

@coveralls

This comment has been minimized.

coveralls commented Jul 24, 2016

Coverage Status

Coverage remained the same at 82.908% when pulling 2ebe0f6 on ghoshbishakh:docsPy3 into c09a194 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 24, 2016

Coverage Status

Coverage remained the same at 82.908% when pulling 0775018 on ghoshbishakh:docsPy3 into c09a194 on nipy:master.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 24, 2016

That PR solved that issue @MarcCote 😄
But now I am getting another error:

Computing tensors...
Computing worst-case/best-case SNR using the corpus callosum...
Noise standard deviation sigma=  9.83112897825
SNR for the b=0 image is : 42.0959119804
SNR for direction 58   [ 0.98875  0.1177  -0.09229] is : 5.47338194103
SNR for direction 57   [-0.05039    0.99871    0.0054406] is : 23.9479488745
SNR for direction 126   [-0.11825  -0.039925  0.99218 ] is : 24.0116089426
streamline_formats.py
/home/bishakh/projects/dipy/dipy/data/files/tracks300.trk
Traceback (most recent call last):
  File "../tools/make_examples.py", line 122, in <module>
    exec(open(script).read(), namespace)
  File "<string>", line 54, in <module>
  File "/home/bishakh/projects/dipy/dipy/io/dpy.py", line 70, in __init__
    ['0.0.1'], 'Dpy Version Number')
  File "/usr/lib/python3.5/site-packages/tables/_past.py", line 35, in oldfunc
    return obj(*args, **kwargs)
  File "/usr/lib/python3.5/site-packages/tables/file.py", line 1152, in create_array
    obj=obj, title=title, byteorder=byteorder)
  File "/usr/lib/python3.5/site-packages/tables/array.py", line 188, in __init__
    byteorder, _log)
  File "/usr/lib/python3.5/site-packages/tables/leaf.py", line 262, in __init__
    super(Leaf, self).__init__(parentnode, name, _log)
  File "/usr/lib/python3.5/site-packages/tables/node.py", line 267, in __init__
    self._v_objectid = self._g_create()
  File "/usr/lib/python3.5/site-packages/tables/array.py", line 205, in _g_create
    raise TypeError("Array objects cannot currently deal with void, "
TypeError: Array objects cannot currently deal with void, unicode or object arrays
Closing remaining open files:fornix.dpy...done
Makefile:122: recipe for target 'rstexamples-stamp' failed
make: *** [rstexamples-stamp] Error 1
make html  1588.85s user 1668.44s system 216% cpu 25:02.59 total

screenshot from 2016-07-24 20-26-25

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 24, 2016

Here is the output if I run streamline_formats.py separately:

bishakh@bisu-arch ~/projects/dipy/doc/examples (git)-[docsPy3] % python streamline_formats.py 
/home/bishakh/projects/dipy/dipy/data/files/tracks300.trk
Traceback (most recent call last):
  File "streamline_formats.py", line 54, in <module>
    dpw = Dpy('fornix.dpy', 'w')
  File "/home/bishakh/projects/dipy/dipy/io/dpy.py", line 70, in __init__
    ['0.0.1'], 'Dpy Version Number')
  File "/usr/lib/python3.5/site-packages/tables/_past.py", line 35, in oldfunc
    return obj(*args, **kwargs)
  File "/usr/lib/python3.5/site-packages/tables/file.py", line 1152, in create_array
    obj=obj, title=title, byteorder=byteorder)
  File "/usr/lib/python3.5/site-packages/tables/array.py", line 188, in __init__
    byteorder, _log)
  File "/usr/lib/python3.5/site-packages/tables/leaf.py", line 262, in __init__
    super(Leaf, self).__init__(parentnode, name, _log)
  File "/usr/lib/python3.5/site-packages/tables/node.py", line 267, in __init__
    self._v_objectid = self._g_create()
  File "/usr/lib/python3.5/site-packages/tables/array.py", line 205, in _g_create
    raise TypeError("Array objects cannot currently deal with void, "
TypeError: Array objects cannot currently deal with void, unicode or object arrays
Closing remaining open files:fornix.dpy...done
@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 24, 2016

looks like changing

self.version = self.f.createArray(self.f.root, 'version',
                                              ['0.0.1'], 'Dpy Version Number')

to

self.version = self.f.createArray(self.f.root, 'version',
                                              [b"0.0.1"], 'Dpy Version Number')

fixes it.

Got some clue from here:
PyTables/PyTables@b7c1430

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 27, 2016

@arokem can you suggest me if I should make that change in the string in this PR?

@arokem

This comment has been minimized.

Member

arokem commented Jul 27, 2016

Where is this change implemented? In dipy/io/dpy.py? Does it work with Python 2 when you implement this change?

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 28, 2016

@arokem yes the change is in dipy/io/dpy.py

And yes it works in both python2 and 3 and the output is:

bishakh@bisu-arch ~/projects/dipy/doc/examples (git)-[docsPy3] % python2 streamline_formats.py
/home/bishakh/projects/dipy/dipy/data/files/tracks300.trk
300
5
bishakh@bisu-arch ~/projects/dipy/doc/examples (git)-[docsPy3] % python streamline_formats.py 
/home/bishakh/projects/dipy/dipy/data/files/tracks300.trk
300
5
@arokem

This comment has been minimized.

Member

arokem commented Jul 28, 2016

Then yes - I say we should include it in this PR. @Garyfallidis : any thoughts here?

@arokem

This comment has been minimized.

Member

arokem commented Jul 28, 2016

Sorry - let me walk back that last comment. Thinking about this for one more second: I think this should be a separate PR. In particular, I would like to see a test that fails on current master, and is fixed when this change is introduced.

@arokem

This comment has been minimized.

Member

arokem commented Jul 28, 2016

That should make for a really small PR, which we can merge quickly. Then, you can rebase this current PR on top of that change and move ahead with it. How does that sound?

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 28, 2016

@arokem Ok I am making a separate PR quickly

@arokem

This comment has been minimized.

Member

arokem commented Jul 29, 2016

OK - now that #1100 is merged, you can rebase this one and keep going here.

@ghoshbishakh ghoshbishakh force-pushed the ghoshbishakh:docsPy3 branch from 0775018 to 37fc41e Jul 29, 2016

@coveralls

This comment has been minimized.

coveralls commented Jul 29, 2016

Coverage Status

Coverage remained the same at 82.908% when pulling 37fc41e on ghoshbishakh:docsPy3 into 63f6a5f on nipy:master.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 31, 2016

Now there is a problem with restore_dti.py

restore_dti.py
Traceback (most recent call last):
  File "../tools/make_examples.py", line 122, in <module>
    exec(open(script).read(), namespace)
  File "<string>", line 39, in <module>
NameError: name 'reload' is not defined
make: *** [Makefile:122: rstexamples-stamp] Error 1
make html  6893.82s user 29.25s system 244% cpu 47:13.14 total
@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 31, 2016

except for issue #1104 everything works fine now! I have made a PR #1105 to solve that.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 31, 2016

@arokem now this works with all examples enabled except :

  1. linear_fascicle_evaluation.py
  2. reconst_dki.py

I have not checked for these two

@arokem

This comment has been minimized.

Member

arokem commented Jul 31, 2016

Why are you not running these? The DKI example does rely on downloading a large amount of data (we need data with multiple b-values...). I just ran it on Python 3 on my laptop and it runs fine.

The LiFE example (linear_fascicle_evalutation.py) doesn't require large amounts of data, or large amounts of RAM, so you should try running it. I can't get it to run on my machine, because I don't have VTK installed on my Python 3 installation, but if you have VTK installed, you should try running that.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 31, 2016

I ran LiFE example and it does take a lot of ram and my pc is stuck now. I
will let it run for tonight and check the result tomorrow morning.

Just out of curiosity.. I noticed some examples use all the 4 cores to
process and run considerably fast while some others make use of only one
core and take a lot of time.
Why is this and can this be improved?

On Jul 31, 2016 11:41 PM, "Ariel Rokem" notifications@github.com wrote:

Why are you not running these? The DKI example does rely on downloading a
large amount of data (we need data with multiple b-values...). I just ran
it on Python 3 on my laptop and it runs fine.

The LiFE example (linear_fascicle_evalutation.py) doesn't require large
amounts of data, or large amounts of RAM, so you should try running it. I
can't get it to run on my machine, because I don't have VTK installed on my
Python 3 installation, but if you have VTK installed, you should try
running that.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1091 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AE6vs6vhJ3dcYDiEp0GIcD6h159qn9JCks5qbOVpgaJpZM4I_8U0
.

@arokem

This comment has been minimized.

Member

arokem commented Jul 31, 2016

Did you run the streamline_tools example beforehand?

If you have the file 'lr-superiorfrontal.trk' in place already (that is,
after tracking) the LiFE example takes (on my machine) about 3 minutes to
run, and takes about 38 MB RAM.

As for full utilization of cpu: some of our code already uses
multiprocessing, and some still needs to be rewritten to use
multiprocessing :-)

On Sun, Jul 31, 2016 at 11:33 AM, Bishakh Ghosh notifications@github.com
wrote:

I ran LiFE example and it does take a lot of ram and my pc is stuck now. I
will let it run for tonight and check the result tomorrow morning.

Just out of curiosity.. I noticed some examples use all the 4 cores to
process and run considerably fast while some others make use of only one
core and take a lot of time.
Why is this and can this be improved?

On Jul 31, 2016 11:41 PM, "Ariel Rokem" notifications@github.com wrote:

Why are you not running these? The DKI example does rely on downloading a
large amount of data (we need data with multiple b-values...). I just ran
it on Python 3 on my laptop and it runs fine.

The LiFE example (linear_fascicle_evalutation.py) doesn't require large
amounts of data, or large amounts of RAM, so you should try running it. I
can't get it to run on my machine, because I don't have VTK installed on
my
Python 3 installation, but if you have VTK installed, you should try
running that.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1091 (comment), or mute
the thread
<
https://github.com/notifications/unsubscribe-auth/AE6vs6vhJ3dcYDiEp0GIcD6h159qn9JCks5qbOVpgaJpZM4I_8U0

.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1091 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHPNo7mIEWi3jr1iKPVejbaXXvP_drWks5qbOptgaJpZM4I_8U0
.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Aug 1, 2016

@arokem I just ran the example separately this time and it worked.

bishakh@bisu-arch ~/projects/dipy/doc/examples (git)-[docsPy3] % python linear_fascicle_evaluation.py
Dataset is already in place. If you want to fetch it again please first remove the folder /home/bishakh/.dipy/stanford_hardi 
Dataset is already in place. If you want to fetch it again please first remove the folder /home/bishakh/.dipy/stanford_hardi 
Dataset is already in place. If you want to fetch it again please first remove the folder /home/bishakh/.dipy/stanford_hardi 
Dataset is already in place. If you want to fetch it again please first remove the folder /home/bishakh/.dipy/stanford_hardi 
Dataset is already in place. If you want to fetch it again please first remove the folder /home/bishakh/.dipy/stanford_hardi 
Camera Position (40.00,52.50,330.80)
Camera Focal Point (40.00,52.50,37.50)
Camera View Up (0.00,1.00,0.00)
-------------------------------------
Camera New Position (-1.00,0.00,0.00)
Camera New Focal Point (0.00,0.00,0.00)
Camera New View Up (0.00,0.00,1.00)
python linear_fascicle_evaluation.py  285.87s user 16.08s system 19% cpu 25:44.50 total
@arokem

This comment has been minimized.

Member

arokem commented Aug 1, 2016

Excellent! I think this is probably ready to go then, right?

Anyone else want to take a look?

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Aug 1, 2016

@arokem yes it is working perfectly. Let others take a look for a couple of days.

@arokem arokem merged commit e309c15 into nipy:master Aug 2, 2016

4 checks passed

codecov/patch Coverage not affected when comparing 63f6a5f...37fc41e
Details
codecov/project 80.84% (+0.00%) compared to 63f6a5f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 82.908%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment