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

Added Examples #1584

Merged
merged 10 commits into from Sep 8, 2018
Merged

Added Examples #1584

merged 10 commits into from Sep 8, 2018

Conversation

karandeepSJ
Copy link
Contributor

This PR adds examples for the new elements in viz.ui. Currently, this includes the examples for the merged elements. I will add more as the other PRs get merged.

@pep8speaks
Copy link

pep8speaks commented Jul 6, 2018

Hello @karandeepSJ, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 12, 2018 at 15:09 Hours UTC

@dmreagan dmreagan added this to PR needs a review in Viz Module Jul 17, 2018
@dmreagan
Copy link
Contributor

I'm not sure we want a second UI example. I think I'd prefer to clean up the existing example and add these to it. By clean up, I mean:

  • make the window larger so you can see everything
  • line all the elements up in a nice grid

Something like this

@codecov-io
Copy link

codecov-io commented Aug 12, 2018

Codecov Report

Merging #1584 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1584      +/-   ##
==========================================
+ Coverage   87.33%   87.34%   +0.01%     
==========================================
  Files         246      246              
  Lines       32177    32265      +88     
  Branches     3495     3504       +9     
==========================================
+ Hits        28102    28183      +81     
- Misses       3242     3247       +5     
- Partials      833      835       +2
Impacted Files Coverage Δ
dipy/viz/ui.py 89.2% <ø> (-0.01%) ⬇️
dipy/viz/tests/test_ui.py 82.14% <0%> (-0.4%) ⬇️
dipy/viz/actor.py 82.79% <0%> (+0.01%) ⬆️
dipy/viz/tests/test_actors.py 74.18% <0%> (+0.28%) ⬆️
dipy/workflows/tests/test_workflow.py 88.63% <0%> (+0.4%) ⬆️
dipy/viz/window.py 64.8% <0%> (+0.57%) ⬆️
dipy/workflows/multi_io.py 68.7% <0%> (+1.5%) ⬆️
dipy/viz/utils.py 61.67% <0%> (+5.98%) ⬆️

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 4890903...3170c80. Read the comment docs.

@karandeepSJ
Copy link
Contributor Author

New Examples:
examples

@dmreagan
Copy link
Contributor

Huge improvement over the previous mess! Ultimately we'll want this to replace viz_ui.py instead of adding a second file. We'll also want to edit the text so it provides a coherent tutorial for the user.

@skoudoro
Copy link
Member

skoudoro commented Sep 6, 2018

I feel like this PR is ready to go. Any other comment? I will wait until tomorrow for merging it

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @karandeepSJ, it is almost ready to be merged. Can you fix the small comment below.

Thank you


First, a bunch of imports.

"""
import os
Copy link
Member

Choose a reason for hiding this comment

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

can you separate """ and import os by an empty line.

We need it to render the documentation correctly

@skoudoro
Copy link
Member

skoudoro commented Sep 8, 2018

Thank you @karandeepSJ and @dmreagan. Merging!

@skoudoro skoudoro merged commit 98495ba into dipy:master Sep 8, 2018
Viz Module automation moved this from PR needs a review to Done Sep 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Viz Module
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants