Skip to content

[ENH] Refactor VTK and tvtk -based interfaces #973

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 80 commits into from
Feb 22, 2016

Conversation

oesteban
Copy link
Contributor

This PR has ended up as follows:

  • Improve the way VTK and tvtk are loaded (created nipype.interfaces.vtkbase to encapsulate it).
  • Improve the use of VTK and tvtk in algorithms.mesh and interfaces.fsl.utils.
  • Automate outputs/inputs decision depending on VTK version (see AttributeError: 'PolyDataWriter' object has no attribute 'set_input_data' #1218)
  • Set ETS_TOOLKIT=null (headless operation) before loading tvtk, then leave environment as it was before (close Enthought Tool Suite configuration #972).
  • Added VTK and tvtk in travis for python 2.7 and in circleCi. AFAIK, VTK wrapping is not quite ready in python 3.x. Before this, both travis and circle skipped tests because VTK was not found (tests had to be run locally).

@oesteban
Copy link
Contributor Author

Hi @satra, I think this is now like the matplotlib backend configuration but it doesn't look like working out... It seems it's not set when pipelines are included in the __main__ of a script.

try:
import matplotlib
matplotlib.use('%s')
except ImportError:
can_import_matplotlib = False
pass
except
Copy link
Member

Choose a reason for hiding this comment

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

what's with this lone "except"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's ugly :( (deleting...).

Thanks for spotting it :)

@chrisgorgo
Copy link
Member

Could you add info about this variable to the docs? http://nipy.org/nipype/users/config_file.html

@oesteban
Copy link
Contributor Author

oesteban commented Apr 9, 2015

I would not merge this though, I think they are changing this in ETS.

I have to check before merging.

@chrisgorgo
Copy link
Member

any updates on this?

@oesteban
Copy link
Contributor Author

I think I'm closing this PR. First of all, in ETS this 'null' plugin is changing its place. Second, even with this config, initialization was not working ok.

If somebody else reports problem with ETS, then I will check on this again.

oesteban added a commit to oesteban/nipype that referenced this pull request Sep 18, 2015
@oesteban oesteban changed the title Added ets_toolkit config option [ENH] Prevent crash when tvtk is loaded ETS_TOOLKIT null Sep 24, 2015
Conflicts:
	CHANGES
	nipype/algorithms/mesh.py
	nipype/algorithms/tests/test_mesh_ops.py
satra added a commit to satra/nipype that referenced this pull request Dec 12, 2015
* upstream/master:
  Correct versions and consistency in install.rst
  fix:import utils needed one more dot
  fix: absolute to relative imports
  add vtk version checking in fsl interfaces
  update CHANGES
  Fix nipy#1218 only, not addressing nipy#973
  force setting ETS_TOOLKIT before tvtk nipy#972
  Created TVTKBaseInterface. Should fix nipy#1218
desc=('Name of file containing initial warp-fields/coefficients (follows premat). This could e.g. be a '
'fnirt-transform from a subjects structural scan to an average of a group '
'of subjects.'))
desc='Name of file containing initial warp-fields/coefficients (follows premat). This could e.g. be a '
Copy link
Member

Choose a reason for hiding this comment

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

are you sure, this works? without the parenthesis? can you check the doc rendering as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oesteban
Copy link
Contributor Author

Hi @satra, I've been fighting with the settings of VTK6 and the mayavi version you suggested. I am reaching to the conclusion that the effort is not worth (yet). Caveats:

  1. VTK6 is not in precise (default release of ubuntu in travis), so I had to update to trusty (which is in testing phase, maybe we don't want to have testing build environments).
  2. Kitware does not support python 3 (see Python 3 support enthought/mayavi#84), therefore we are in the exact same situation as before.

We can move on trusty, and simplify the travis settings, but anyways it has to be on python 2. Should I revert back the code (in the end, it was working before), save the current config to a gist and update it when VTK supports python 3?

@oesteban
Copy link
Contributor Author

@satra, I finally reverted back the travis config file. I think we should wait for support of python 3 in VTK. In the meantime, this is good to check that it works also without VTK

@satra
Copy link
Member

satra commented Feb 13, 2016

some conflicts have risen with all the code that has been merged.

@oesteban - do you mean we need to check that it works without VTK? or that you have already checked?

@oesteban
Copy link
Contributor Author

When python is 2 then the vtk is installed and explicit regression tests (no auto tests) are done. If python is 3 vtk is not installed, and we test that this does not cause any problem.

@oesteban
Copy link
Contributor Author

Is this ready to go? I would need to merge this before the inputs/outputs refactoring.

I finally stalled #1240 and continued outside that PR because the changes were huge. If you want to have a sneak peek on how things are, I am working on this branch: https://github.com/oesteban/nipype/tree/enh/DerivationOfInterfacesRefactoring. This branch could mean a jump to v. 1.5 or similar (I would reserve 2.0 for a version including the workflow patterns).

If this branch was merged, then I will withdraw #1240 and #882 since they will not make any sense (for the interfaced workflows, with the new code it will be rather easy to produce the pattern).

satra added a commit that referenced this pull request Feb 22, 2016
[ENH] Refactor VTK and tvtk -based interfaces
@satra satra merged commit 47587b9 into nipy:master Feb 22, 2016
@satra
Copy link
Member

satra commented Feb 22, 2016

@oesteban - we have merged a bunch of PRs without making any changes to the CHANGES file. can we update it for at least the major ones.

@oesteban oesteban deleted the enh/ETSConfigTookit branch September 8, 2016 18:49
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.

Enthought Tool Suite configuration
3 participants