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

Updating the documentation for the workflow creation tutorial. #1520

Merged
merged 3 commits into from May 17, 2018

Conversation

Projects
None yet
4 participants
@parichit
Copy link
Contributor

parichit commented May 14, 2018

  1. This change will be reflected on the page http://nipy.org/dipy/examples_built/workflow_creation.html#example-workflow-creation
  2. Added one more import statement to import the AppendTextFlow method from the workflow class.
  3. Added few lines to explain why we need to import the methods.

Added the desired import statement into the workflow_creation.py file.

The original tutorial mentions that the lines need to be added in a separate file in the "bin" directory but did not mention that this will also require importing the AppendTextFlow method from the newly created class.

Tested the file in bin directory after adding the import statement and the script executes perfectly.

Parichit Sharma
Updating the documentation for the workflow creation tutorial.
1) This change will be reflected on the page http://nipy.org/dipy/examples_built/workflow_creation.html#example-workflow-creation
2) Added one more import statement to import the AppendTextFlow method from the workflow class.
3) Added few lines to explain why we need to import the methods.
@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented May 14, 2018

Hello @parichit, Thank you for updating !

Line 16:81: E501 line too long (110 > 80 characters)
Line 101:81: E501 line too long (84 > 80 characters)
Line 102:81: E501 line too long (81 > 80 characters)
Line 106:1: E402 module level import not at top of file

Comment last updated on May 16, 2018 at 01:26 Hours UTC
Parichit Sharma
Removed the trailing whitespaces as per the PEP8 standard.
1) The PEP8 still complains about the maximum line width but no complaints about the
trailing white spaces.
@parichit

This comment has been minimized.

Copy link
Contributor

parichit commented May 14, 2018

Edited the workflow_creation.py file to resolve the PEP8 warnings about the trailing whitespaces.

However, doing so has created another warning about the maximum width of the line. It is more than 79 characters now. I noticed that several lines are more than 79 characters in width in the workflow_creation.py file so I kept my changes that way to solve the trailing whitespaces warning.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 14, 2018

Codecov Report

Merging #1520 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1520      +/-   ##
==========================================
- Coverage   87.55%   87.55%   -0.01%     
==========================================
  Files         246      246              
  Lines       31366    31366              
  Branches     3433     3433              
==========================================
- Hits        27464    27463       -1     
  Misses       3085     3085              
- Partials      817      818       +1
Impacted Files Coverage Δ
dipy/core/graph.py 73.8% <0%> (-1.2%) ⬇️

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 f4c7e45...34937d4. Read the comment docs.

@skoudoro skoudoro added the gsoc2018 label May 14, 2018

@skoudoro
Copy link
Member

skoudoro left a comment

Hi @parichit, Thank you for this update!

These lines will import the required files and classes into the python file. The
first line imports the run_flow method from the flow_runner class and the second
line imports the AppendTextFlow method from the newly created workflow class.

This comment has been minimized.

@skoudoro

skoudoro May 16, 2018

Member
  • I think the first sentence is not necessary (These lines... python files.)
  • AppendTextFlow method -> AppendTextFlow class

This comment has been minimized.

@parichit

parichit May 16, 2018

Contributor

Hey, thanks.

I removed the first line and changed the comments to explain more clearly.

from dipy.workflows.flow_runner import run_flow
from dipy.workflows.AppendTextFlow import AppendTextFlow

This comment has been minimized.

@skoudoro

skoudoro May 16, 2018

Member

Can you precise above that we create a file named my_workflow.py in dipy/workflows folder ?
After that, Can you change from dipy.workflows.AppendTextFlow import AppendTextFlow to from dipy.workflows.my_workflow import AppendTextFlow ?

This comment has been minimized.

@parichit

parichit May 16, 2018

Contributor

Hey Serge, thanks for going through the changes.

Made the change to

from dipy.workflows.my_workflow import AppendTextFlow

Parichit Sharma
Updated the workflow_creation.py according to the recommendations mad…
…e by Serge

1) Slightly modified the comments to tell that we are creating a separate file called as my_workflow.py in <dipy/workflows> folder.
2) Removed the first line about the import statements.
3) Comments are more precise now.
@skoudoro
Copy link
Member

skoudoro left a comment

Great! Looks good to me.

I will wait for other feedback until Wednesday evening and then merge it.

@skoudoro skoudoro merged commit 6bee25c into nipy:master May 17, 2018

2 of 3 checks passed

codecov/project 87.55% (-0.01%) compared to f4c7e45
Details
codecov/patch Coverage not affected when comparing f4c7e45...34937d4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1520 from parichit/doc_branch
Updating the documentation for the workflow creation tutorial.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment