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

From tables to h5py #1351

Merged
merged 15 commits into from Oct 13, 2017

Conversation

Projects
None yet
4 participants
@Garyfallidis
Member

Garyfallidis commented Oct 7, 2017

Switching from pytables to h5py. H5py still works with older numpy versions. pytables needs newer versions. Therefore h5py is easier to depend on. Also it seems that h5py gives better access to low level functions. Finally, testing in travis should be much simpler without the pytables dependency as that api has been changing quite a bit.

@Garyfallidis Garyfallidis changed the title from WIP: Tables to h5py to WIP: From tables to h5py Oct 7, 2017

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 7, 2017

Still a bit more work to do. But there is progress.

@Garyfallidis Garyfallidis changed the title from WIP: From tables to h5py to From tables to h5py Oct 8, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Oct 8, 2017

Codecov Report

Merging #1351 into master will increase coverage by <.01%.
The diff coverage is 92.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1351      +/-   ##
==========================================
+ Coverage   87.02%   87.03%   +<.01%     
==========================================
  Files         228      228              
  Lines       28841    29034     +193     
  Branches     3100     3128      +28     
==========================================
+ Hits        25099    25269     +170     
- Misses       3037     3058      +21     
- Partials      705      707       +2
Impacted Files Coverage Δ
dipy/workflows/tests/test_reconst_csa_csd.py 96.07% <ø> (-0.22%) ⬇️
dipy/info.py 100% <100%> (ø) ⬆️
dipy/io/dpy.py 100% <100%> (+5.97%) ⬆️
dipy/io/tests/test_io_peaks.py 96.84% <33.33%> (+7.02%) ⬆️
dipy/io/tests/test_dpy.py 93.33% <81.81%> (-6.67%) ⬇️
dipy/io/peaks.py 95.34% <94.87%> (+5.55%) ⬆️
dipy/testing/__init__.py 77.77% <0%> (-6.84%) ⬇️
dipy/viz/ui.py 90.24% <0%> (-2.11%) ⬇️
dipy/viz/tests/test_ui.py 83.53% <0%> (-1.7%) ⬇️
dipy/workflows/tests/workflow_tests_utils.py 100% <0%> (ø) ⬆️
... and 9 more

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 76d021a...80d0a50. Read the comment docs.

Garyfallidis added some commits Oct 9, 2017

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 11, 2017

@arokem I have only one bot failing now. The one with export DEPENDS="". What happens when DEPENDS is empty?. It installs minimum requirements by looking at requirements.txt or info.py or nothing? Can you check my .travis.yml file and suggest solutions?

@arokem

This comment has been minimized.

Member

arokem commented Oct 11, 2017

@skoudoro

This comment has been minimized.

Member

skoudoro commented Oct 11, 2017

@Garyfallidis, In this bot, INSTALL_TYPE="setup" so I think it does not use requirement.txt but you should add a SetupDependency in setup.py like you can see in this link: https://github.com/nipy/dipy/blob/master/setup.py#L150.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 11, 2017

Oh!! Good catch @skoudoro ! :) Let me fix this.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 11, 2017

Done. Let's wait for travis. I am not sure if the parameter heavy should be False or True. The default option is False (and used for nibabel too) so I kept that. If anyone has other suggestions. Let me know.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 11, 2017

👍 we are game! Merge if you think it's good.

@arokem

Looks good. I had a couple of questions/comments.

@@ -150,6 +150,9 @@
SetupDependency('nibabel', info.NIBABEL_MIN_VERSION,
req_type='install_requires',
heavy=False).check_fill(extra_setuptools_args)
SetupDependency('h5py', info.H5PY_MIN_VERSION,

This comment has been minimized.

@arokem

arokem Oct 12, 2017

Member

I don't understand this. Why are we not using the requirements file to set up requirements that don't make crazy loops (such as numpy) ?

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 13, 2017

Member

I just went with what we have done for nibabel here. You may want to chat with @matthew-brett about this. He most likely used this for a good reason.

@@ -7,9 +7,9 @@ Dependencies
Depends on a few standard libraries: python_ (the core language), numpy_ (for
numerical computation), scipy_ (for more specific mathematical operations),
cython_ (for extra speed) and nibabel_ (for file formats; we require version 2.1
or higher). Optionally, it can use python-vtk_ (for visualisation), pytables_
or higher). Optionally, it can use python-vtk_ (for visualisation), h5py_

This comment has been minimized.

@arokem

arokem Oct 12, 2017

Member

Not an optional dependency, is it?

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 13, 2017

Member

Nope, corrected.

(for handling large datasets), matplotlib_ (for scientific plotting), and
ipython_ (for interaction with the code and its results). cvxopt_ (version
1.1.7 or later) is required for some modules.
ipython_ (for interaction with the code and its results). cvxpy is required for

This comment has been minimized.

@arokem

arokem Oct 12, 2017

Member

Good catch!

This comment has been minimized.

@Garyfallidis

@arokem arokem merged commit 0964732 into nipy:master Oct 13, 2017

3 checks passed

codecov/patch 92.5% of diff hit (target 87.02%)
Details
codecov/project 87.03% (+<.01%) compared to 76d021a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arokem

This comment has been minimized.

Member

arokem commented Oct 13, 2017

@arokem

This comment has been minimized.

Member

arokem commented on dipy/io/dpy.py in 72edd97 Oct 24, 2017

This causes #1366

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment