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

From dipy.viz to FURY #1659

Merged
merged 9 commits into from Nov 16, 2018
Merged

From dipy.viz to FURY #1659

merged 9 commits into from Nov 16, 2018

Conversation

skoudoro
Copy link
Member

@skoudoro skoudoro commented Oct 29, 2018

  • Remove fvtk module
  • dipy.viz give birth to FURY --> New Scientific visualization library

More information about FURY:

@codecov-io
Copy link

codecov-io commented Oct 31, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@c0daaeb). Click here to learn what that means.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1659   +/-   ##
=========================================
  Coverage          ?   88.45%           
=========================================
  Files             ?      233           
  Lines             ?    29233           
  Branches          ?     3217           
=========================================
  Hits              ?    25859           
  Misses            ?     2704           
  Partials          ?      670
Impacted Files Coverage Δ
dipy/data/__init__.py 83% <ø> (ø)
dipy/data/fetcher.py 33.24% <ø> (ø)
dipy/viz/__init__.py 90% <100%> (ø)
dipy/io/vtk.py 14.58% <75%> (ø)

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 c0daaeb...6ecd63d. Read the comment docs.

@arokem
Copy link
Contributor

arokem commented Nov 9, 2018

OK. This all looks good to me.

Do you want to add some documentation here pointing to Fury? I think that you mention it where we previously pointed to VTK, but I would add a link there, so that people can easily find it.

@skoudoro
Copy link
Member Author

skoudoro commented Nov 9, 2018

Do you want to add some documentation here pointing to Fury?

I do not understand. Do you mean adding this 2 links:

or something else?

@arokem
Copy link
Contributor

arokem commented Nov 9, 2018 via email

@skoudoro
Copy link
Member Author

skoudoro commented Nov 9, 2018

oh ok, It is not, you are right. I do not know yet where to put it but will do

@arokem
Copy link
Contributor

arokem commented Nov 9, 2018 via email

dipy/io/vtk.py Outdated
@@ -15,8 +13,37 @@
major_version = vtk.vtkVersion.GetVTKMajorVersion()


def set_input(vtk_object, inp):
Copy link
Contributor

Choose a reason for hiding this comment

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

@skoudoro remind me something this function stays in DIPY because we want to have surface loading support? Or there is some other reason? I thought the idea was to remove direct dependencies to vtk. And only indirectly depend through FURY?

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way this is not a suggestion to move io to FURY right now. But something to think for release 0.16.

@Garyfallidis
Copy link
Contributor

+1 for @arokem suggestion to update documentation (and installation instructions) with Fury.
Otherwise this look ready! :)

@skoudoro
Copy link
Member Author

Thanks @arokem and @Garyfallidis for the review. I added the documentation link and fix io/vtk.

It looks like it is ready to go! Can you have a last check?

@Garyfallidis Garyfallidis merged commit a7bb518 into dipy:master Nov 16, 2018
@Garyfallidis
Copy link
Contributor

Great work! To Inf+!

@skoudoro skoudoro deleted the to-fury branch December 3, 2018 16:17
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.

None yet

4 participants