Skip to content

RF: minc - delay import of h5py until needed. #889

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

Merged
merged 1 commit into from
Feb 14, 2020

Conversation

yarikoptic
Copy link
Member

On Debian systems h5py comes with MPI enabled build.
I guess on some misconfigured systems, like my laptop, it might take awhile to import.

And since h5py here is imported upon import of nibabel, it makes
it long(er) to import it. Here is timing on my laptop before the change:

$> time python -c 'import nibabel'
python -c 'import nibabel'  0.20s user 0.08s system 5% cpu 5.356 total

and here is after the change:

$> time python -c 'import nibabel'
python -c 'import nibabel'  0.13s user 0.02s system 100% cpu 0.150 total

On Debian systems h5py comes with MPI enabled build.
I guess on some misconfigured systems, like my laptop, it might take awhile to import.

And since h5py here is imported upon import of `nibabel`, it makes
it long(er) to import it.  Here is timing on my laptop before the change:

    $> time python -c 'import nibabel'
    python -c 'import nibabel'  0.20s user 0.08s system 5% cpu 5.356 total

and here is after the change:

    $> time python -c 'import nibabel'
    python -c 'import nibabel'  0.13s user 0.02s system 100% cpu 0.150 total
@codecov
Copy link

codecov bot commented Feb 14, 2020

Codecov Report

Merging #889 into master will decrease coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #889      +/-   ##
==========================================
- Coverage   89.55%   89.38%   -0.17%     
==========================================
  Files          96       96              
  Lines       12378    12378              
  Branches     2184     2184              
==========================================
- Hits        11085    11064      -21     
- Misses        944      966      +22     
+ Partials      349      348       -1     
Impacted Files Coverage Δ
nibabel/keywordonly.py 92.85% <0.00%> (-7.15%) ⬇️
nibabel/nicom/dicomwrappers.py 85.16% <0.00%> (-5.75%) ⬇️
nibabel/filename_parser.py 92.04% <0.00%> (-2.28%) ⬇️
nibabel/volumeutils.py 84.13% <0.00%> (+0.19%) ⬆️
nibabel/nifti1.py 91.51% <0.00%> (+0.30%) ⬆️
nibabel/pydicom_compat.py 77.27% <0.00%> (+13.63%) ⬆️

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 bcce691...bb9cb15. Read the comment docs.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

LGTM.

@effigies
Copy link
Member

Can't see any reason to delay it. I'll also cherry-pick into maint/3.0.x so that it will be in the next bug-fix release.

@effigies effigies merged commit 29bdb59 into nipy:master Feb 14, 2020
@yarikoptic
Copy link
Member Author

THANK YOU @effigies !

@yarikoptic
Copy link
Member Author

I am a bit confused by coverage report, ... but oh well -- it is not the first time this happens to me ;)

@effigies
Copy link
Member

Yeah, it can be a mess.

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.

2 participants