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

[Fix] Workflow mask documentation #1835

Merged
merged 2 commits into from Jun 12, 2019

Conversation

@skoudoro
Copy link
Member

commented May 16, 2019

As discussed with @jchoude, the goal of this PR is to fix #1764. The docstring was not coherent with the current behavior. Indeed, mask is mandatory for all reconstruction workflow.

@pep8speaks

This comment has been minimized.

Copy link

commented May 16, 2019

Hello @skoudoro, Thank you for updating !

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

Comment last updated at 2019-05-16 19:24:22 UTC
@arokem

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Hmm. This fixes the documentation, but doesn't implement your suggestion, so I don't think it really resolves the issue.

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

I misunderstood the original issue. The original issue was just a misalignment between the workflow behavior and the documentation.

More generally, mask_files is mandatory for many reconstruction workflows. Indeed, it does not make a sense to fit the background on many reconstructions. To have a design coherence between all reconstruction workflow, I think it is a really bad idea to turn mask_files optional for DTI. I believe we should educate our users to provide a mask and not run a reconstruction without it. So my previous suggestion/question does not make any more sense and the current behavior is good.

@codecov-io

This comment has been minimized.

Copy link

commented May 16, 2019

Codecov Report

Merging #1835 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1835   +/-   ##
=======================================
  Coverage   84.28%   84.28%           
=======================================
  Files         117      117           
  Lines       14222    14222           
  Branches     2245     2245           
=======================================
  Hits        11987    11987           
  Misses       1718     1718           
  Partials      517      517
Impacted Files Coverage Δ
dipy/workflows/reconst.py 77.22% <ø> (ø) ⬆️
@arokem

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Yep yep. Mea culpa for the long wait.

@arokem arokem merged commit 703cd83 into nipy:master Jun 12, 2019

5 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch Coverage not affected when comparing c93f88b...56ada69
Details
codecov/project 84.28% remains the same compared to c93f88b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skoudoro skoudoro deleted the skoudoro:fix-1764 branch Jul 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.