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

Update the way we import external libraries by using only the necessary modules #470

Merged
merged 7 commits into from Sep 21, 2021

Conversation

skoudoro
Copy link
Contributor

@skoudoro skoudoro commented Jul 26, 2021

This PR is the beginning of #457.

The goal is to make our scripts load faster by importing only the functions/modules we need from VTK.

@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

Merging #470 (c3c3386) into master (48939ae) will increase coverage by 0.17%.
The diff coverage is 92.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #470      +/-   ##
==========================================
+ Coverage   89.03%   89.21%   +0.17%     
==========================================
  Files          33       34       +1     
  Lines        7112     7230     +118     
  Branches      836      836              
==========================================
+ Hits         6332     6450     +118     
  Misses        547      547              
  Partials      233      233              
Impacted Files Coverage Δ
fury/__init__.py 65.71% <20.00%> (ø)
fury/io.py 81.01% <72.72%> (-0.12%) ⬇️
fury/window.py 81.90% <77.77%> (-0.05%) ⬇️
fury/utils.py 85.82% <84.00%> (-0.04%) ⬇️
fury/actors/peak.py 89.94% <90.00%> (-0.06%) ⬇️
fury/actor.py 89.62% <90.21%> (-0.02%) ⬇️
fury/actors/odf_slicer.py 87.61% <100.00%> (ø)
fury/actors/tests/test_peak.py 97.95% <100.00%> (ø)
fury/colormap.py 93.89% <100.00%> (ø)
fury/convert.py 80.00% <100.00%> (ø)
... and 11 more

@skoudoro
Copy link
Contributor Author

skoudoro commented Sep 8, 2021

This PR is ready to go. Can you have a look @Garyfallidis and @guaje?

Thank you

@Garyfallidis
Copy link
Contributor

Garyfallidis commented Sep 8, 2021

@skoudoro there is currently a conflict.
I wonder if we should have one file that does all the vtk imports and then just load functions from that file.
For example,

from fury.fvtk import vtkPolyDataMapper

@Garyfallidis
Copy link
Contributor

And then in fvtk.py we add the

import vtkmodules.vtkCommonCore as ccvtk
Polydata = ccvtk.vtkPolyDataMapper
or
vtkPolydata = ccvtk.vtkPolyDataMapper

In this way in the other scripts you can just do

from fury.fvtk import vtkPolyData
or 
from fury.fvtk import Polydata

Also what speedup do you see?

@skoudoro skoudoro changed the title update import in window modules update import by using only the necessary modules Sep 10, 2021
@Garyfallidis Garyfallidis changed the title update import by using only the necessary modules Update the way we import external libraries by using only the necessary modules Sep 21, 2021
@Garyfallidis
Copy link
Contributor

This is great and much needed!
Next steps are:

a) update molecular.py file to use new gig.
b) update all tutorials and demos.

Thank you @skoudoro !

@Garyfallidis Garyfallidis merged commit 39a2803 into fury-gl:master Sep 21, 2021
@Garyfallidis
Copy link
Contributor

@skoudoro on your upcoming PR please also report timings. How much faster is importing in this new way than before?
For example, how much time an application takes to load now and before ?

@skoudoro skoudoro deleted the vtkmodules-windows branch February 3, 2023 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants