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

Base workflow enhancements + tests #1087

Merged
merged 18 commits into from Sep 8, 2016

Conversation

Projects
None yet
7 participants
@matthieudumont
Contributor

matthieudumont commented Jun 23, 2016

Redux version of my previous enhanced_workflows PR.
Cuts the additions in half but it is still pretty big. The way it is made, this is close to the smallest I can get while keeping something coherent enough to be merged by itself. Let me know what you think of the size.

Content:
Enhanced workflows:
-Better command line interface
-Automatic command line parameters for all workflows like logging params and force overwrite.
-Easier to create new workflows.
-Simpler and more consistent ouput management
Combined workflow (pipelines)
-Instantly exposes sub workflows parameters to command line.
Tests for all the workflow/combined workflow mecanics, argparser, flow_runner
Examples on how to create a workflow and a combined workflow.

else:
result_path = out_dir
if not exists(result_path):

This comment has been minimized.

@MarcCote

MarcCote Jun 23, 2016

Contributor

We could easily make the folder creation thread-safe like this:

try:
    makedirs(result_path)
except OSError:
    pass
@@ -0,0 +1,65 @@
from __future__ import division, print_function, absolute_import

This comment has been minimized.

@MarcCote

MarcCote Jun 23, 2016

Contributor

Any particular reason you import absolute_import here?

This comment has been minimized.

@matthieudumont

matthieudumont Jun 23, 2016

Contributor

No, probably copied from somewhere else.

Looking into it, I copied the first import line used across all dipy. Should I remove this or is it a good practice to to that?

class Workflow(object):
def __init__(self, output_strategy='append', mix_names=False,

This comment has been minimized.

@MarcCote

MarcCote Jun 23, 2016

Contributor

Missing docstrings for this class and its methods.

output. Lastly, all out_ params needs to be at the end of the params
list.
The class docstring part is very important, you need to document

This comment has been minimized.

@MarcCote

MarcCote Jun 23, 2016

Contributor

You mean the run method docstring?

This comment has been minimized.

@matthieudumont

matthieudumont Jun 23, 2016

Contributor

Yes, will fix

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 3, 2016

Member

This still needs correction here.

io_it = self.get_io_iterator()
for in_file, out_file in io_it:

This comment has been minimized.

@MarcCote

MarcCote Jun 23, 2016

Contributor

This out_file variable shadows the out_file parameter. Is this expected? If so, I'm guessing the parameter out_file is used when you call self.get_io_iterator() via introspection?

This comment has been minimized.

@matthieudumont

matthieudumont Jun 23, 2016

Contributor

No it is an error on my part from working with generic names for examples.
They should have different names. out_file (as param) is the name of the file you want, what comes from the it_iterator is the full path (for this file).

"""
This is it for the workflow! Now to be able to call it easily via command
line, you need to add this bit of code. Usually this is in a sperate

This comment has been minimized.

@MarcCote

MarcCote Jun 23, 2016

Contributor

Typo: separate

from dipy.workflows.flow_runner import run_flow
"""
This is the method that will wrap everything that is need to make a flow

This comment has been minimized.

@MarcCote

MarcCote Jun 23, 2016

Contributor

Typo: is needed

Workflows to inspect.
"""
sub_flow_optionnals = {}

This comment has been minimized.

@MarcCote

MarcCote Jun 23, 2016

Contributor

Typo: optionnals -> optionals

" 1-18, 2014."
"References: \n" \
"Garyfallidis, E., M. Brett, B. Amirbekian, A. Rokem," \
" S. Van Der Walt, M. Descoteaux, and I. Nimmo-Smith. Dipy, a" \

This comment has been minimized.

@MarcCote

MarcCote Jun 23, 2016

Contributor

I think flake8 will warn about this line (81 chars.). What about this:

            epilog = 
                ("References: \n"
                 "Garyfallidis, E., M. Brett, B. Amirbekian, A. Rokem,"
                 " S. Van Der Walt, M. Descoteaux, and I. Nimmo-Smith. Dipy, a"
                 " library for the analysis of diffusion MRI data. Frontiers"
                 " in Neuroinformatics, 1-18, 2014.")
self.outputs = NumpyDocString(doc)['Outputs']
self.outputs =\

This comment has been minimized.

@MarcCote

MarcCote Jun 23, 2016

Contributor

I think it would read better like this:

        self.outputs = [param for param in npds['Parameters']
                        if 'out_' in param[0]]
"Garyfallidis, E., M. Brett, B. Amirbekian, A. Rokem,"
" S. Van Der Walt, M. Descoteaux, and I. Nimmo-Smith. Dipy, a"
" library for the analysis of diffusion MRI data. Frontiers"
" in Neuroinformatics, 1-18, 2014.")

This comment has been minimized.

@arokem

arokem Jun 25, 2016

Member

Should we use duecredit (https://github.com/duecredit/duecredit/) to manage references?

This comment has been minimized.

@matthieudumont

matthieudumont Jun 27, 2016

Contributor

I definitly wouldnt mind usig it. I suggest making a separate PR for this afterwards.

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 31, 2016

Member

+1 for trying to use duecredit
+1 for doing this at a later stage and not in this PR.

@coveralls

This comment has been minimized.

coveralls commented Jun 27, 2016

Coverage Status

Coverage decreased (-0.2%) to 82.718% when pulling c239abd on matthieudumont:enhanced_workflows_base into 2836d60 on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Jun 27, 2016

Current coverage is 80.70% (diff: 78.45%)

Merging #1087 into master will decrease coverage by 0.26%

@@             master      #1087   diff @@
==========================================
  Files           205        211     +6   
  Lines         23329      23621   +292   
  Methods           0          0          
  Messages          0          0          
  Branches       2341       2401    +60   
==========================================
+ Hits          18889      19064   +175   
- Misses         3958       4058   +100   
- Partials        482        499    +17   

Powered by Codecov. Last update 31bc6fe...024da32

@coveralls

This comment has been minimized.

coveralls commented Jun 27, 2016

Coverage Status

Coverage decreased (-0.2%) to 82.718% when pulling c239abd on matthieudumont:enhanced_workflows_base into 2836d60 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 28, 2016

Coverage Status

Coverage decreased (-0.2%) to 82.725% when pulling 9c9a54b on matthieudumont:enhanced_workflows_base into 2836d60 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 28, 2016

Coverage Status

Coverage decreased (-0.1%) to 82.748% when pulling 8d81b89 on matthieudumont:enhanced_workflows_base into 2836d60 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 29, 2016

Coverage Status

Coverage decreased (-0.1%) to 82.752% when pulling 5451f5d on matthieudumont:enhanced_workflows_base into 2836d60 on nipy:master.

@matthieudumont

This comment has been minimized.

Contributor

matthieudumont commented Jun 29, 2016

bump, finally made is pass on all python versions!

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 4, 2016

So this PR actually improves the process of writing new combined workflows. But all the previous workflows like dipy_fit_tensor, dipy_peak_extraction, dipy_quickbundles, dipy_sh_estimate are removed. Only dipy_median_otsu is improved to use the new workflow.

So will the others be rewritten in this PR?

@matthieudumont

This comment has been minimized.

Contributor

matthieudumont commented Jul 4, 2016

Yes it improves the workflow and combined_workflow creation. All the previous workflows are already rewritten but the PR was too big so I just kept the base class + 1 implementation (medianotsu). The others will be in PR as soon as this pass.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 4, 2016

@matthieudumont understood! thanks! So others will also be similar to dipy_median_otsu.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 20, 2016

@matthieudumont @MarcCote any suggestions when this will be merged or other workflows will be coming?

@matthieudumont

This comment has been minimized.

Contributor

matthieudumont commented Jul 20, 2016

This is a good question @ghoshbishakh. I dont know if the reviews stopped because everything is good and ready to be merged or because they just stopped. I of course would like this to be merged asap so I can push the rest of the PR and have these workflows and the simple creation tools in master for everyone to use.

Idk if @Garyfallidis is back to work but I guess he can give a hand pushing this forward when he will be ready.

@arokem

This comment has been minimized.

Member

arokem commented Jul 20, 2016

Comments have slowed down a bit because we've all been a bit occupied with GSoC. I'll try to take a look some time this week.

specs = inspect.getargspec(workflow)
doc = inspect.getdoc(workflow)
self.doc = NumpyDocString(doc)['Parameters']
""" Take a workflow object and use introspection to extract the parameters,

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 31, 2016

Member

From numpy doc

"""This is the form of a docstring.

It can be spread over several lines.

"""

There is no need to indent on the second line. More here
https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt

def add_sub_flow_args(self, sub_flows):
""" Take a list workflow objects and use introspection to extract the
parameters, types and docstrings of their run method. Only the

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 31, 2016

Member
""" Take a list workflow objects and use introspection to extract the
parameters, types and docstrings of their run method. Only the
...
"""
typestr = _doc[i][1]
dtype, isnarg = self._select_dtype(typestr)
help_msg = ''.join(_doc[i][2])

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 31, 2016

Member

remove space

def _select_dtype(self, text):
""" Analyses a docstring parameter line and returns the good argparser
type.

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 31, 2016

Member

Indent left (and the parameters).

class CombinedWorkflow(Workflow):
def __init__(self, output_strategy='append', mix_names=False,
force=False, skip=False):
""" Workflow that combines multiple workflows.

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 31, 2016

Member

Explain also the term sub flows that is used in the methods.

@matthieudumont matthieudumont force-pushed the matthieudumont:enhanced_workflows_base branch from 541b5ff to 0345ec7 Aug 29, 2016

@matthieudumont

This comment has been minimized.

Contributor

matthieudumont commented Aug 29, 2016

Rebased.

@coveralls

This comment has been minimized.

coveralls commented Aug 29, 2016

Coverage Status

Coverage decreased (-0.2%) to 82.824% when pulling 0345ec7 on matthieudumont:enhanced_workflows_base into 31bc6fe on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Aug 29, 2016

Coverage Status

Coverage decreased (-0.2%) to 82.824% when pulling 0345ec7 on matthieudumont:enhanced_workflows_base into 31bc6fe on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Aug 29, 2016

Coverage Status

Coverage decreased (-0.2%) to 82.812% when pulling deb66d0 on matthieudumont:enhanced_workflows_base into 31bc6fe on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Aug 30, 2016

Coverage Status

Coverage decreased (-0.2%) to 82.812% when pulling 024da32 on matthieudumont:enhanced_workflows_base into 31bc6fe on nipy:master.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Aug 31, 2016

@matthieudumont oh I see you have already fixed the issue 😄 , thanks!

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Sep 8, 2016

Okay there are some spaces missing in some of the docstrings which are visible when you call --help in the commands. But the important thing now is to start playing with the API. Let's hangout next week @matthieudumont and start working on the details. We should also include @ghoshbishakh in the discussion as he is working on enabling cmd docs in the new website. Crack On!

@Garyfallidis Garyfallidis merged commit 5d64822 into nipy:master Sep 8, 2016

1 of 4 checks passed

codecov/patch 78.45% of diff hit (target 80.96%)
Details
codecov/project 80.70% (-0.27%) compared to 31bc6fe
Details
coverage/coveralls Coverage decreased (-0.2%) to 82.812%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Sep 8, 2016

@matthieudumont

This comment has been minimized.

Contributor

matthieudumont commented Sep 8, 2016

Sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment