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

MAINT: Revisions after previous maintenance commit (#83) #85

Merged
merged 3 commits into from
Mar 11, 2020

Conversation

oesteban
Copy link
Member

Revise #83 and fix the two outstanding issues:

  • Outdated requirements.txt file when generating docs.
  • Inconsistent handling of the --output-spaces section, as a result of copy&paste from fMRIPrep.

@pull-assistant
Copy link

pull-assistant bot commented Mar 10, 2020

Score: 0.76

Best reviewed: commit by commit


Optimal code review plan

     docs: update dependencies in docs/requirements.txt

     fix: update the --output-spaces option to be more consistent

     fix(docs,workflow): include @josephmje's suggestions

Powered by Pull Assistant. Last update 3496a2e ... 35abaae. Read the comment docs.

@oesteban oesteban requested a review from josephmje March 10, 2020 20:15
@oesteban oesteban added the bug Something isn't working label Mar 10, 2020
@ghost
Copy link

ghost commented Mar 10, 2020

Warnings are found on analyzing the commit 03b0302.

1 warning:

We recommend to address them as possible, for example, update outdated dependencies, fix the tool's configuration, configure sider.yml, turn off unused tools, and so on.

If you are struggling with these errors or warnings, feel free to ask us via chat. 💬

@oesteban
Copy link
Member Author

for some obscure reason, circle has stopped to build my PRs :(

@codecov
Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #85 into master will increase coverage by 0.24%.
The diff coverage is 22.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
+ Coverage   52.61%   52.85%   +0.24%     
==========================================
  Files          16       16              
  Lines         899      893       -6     
  Branches      110      109       -1     
==========================================
- Hits          473      472       -1     
+ Misses        425      420       -5     
  Partials        1        1
Impacted Files Coverage Δ
dmriprep/workflows/base.py 28.57% <0%> (-1.01%) ⬇️
dmriprep/cli/run.py 31.15% <33.33%> (+0.58%) ⬆️

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 7c33d27...35abaae. Read the comment docs.

@arokem
Copy link
Collaborator

arokem commented Mar 10, 2020

for some obscure reason, circle has stopped to build my PRs :(

That's annoying.

@josephmje : is your PR #81 rebased on current master? Maybe there's a difference between these two PRs that explains why one is tested and the other isn't?

@josephmje
Copy link
Collaborator

for some obscure reason, circle has stopped to build my PRs :(

That's annoying.

@josephmje : is your PR #81 rebased on current master? Maybe there's a difference between these two PRs that explains why one is tested and the other isn't?

Yes, I rebased earlier this morning.

@josephmje
Copy link
Collaborator

josephmje commented Mar 10, 2020

With the new version of smriprep, many of the output_spaces arguments need to be changed to spaces.

eg.

TypeError: init_anat_preproc_wf() got an unexpected keyword argument 'output_spaces'

@josephmje
Copy link
Collaborator

I forgot that the documentation used a different set of package versions. That explains why we were still getting nipype errors.

@oesteban
Copy link
Member Author

@josephmje have you rebased to this one? should I just merge and check whether I successfully addressed all issues?

dmriprep/cli/run.py Outdated Show resolved Hide resolved
@josephmje
Copy link
Collaborator

@josephmje have you rebased to this one? should I just merge and check whether I successfully addressed all issues?

I haven't rebased to this one but I did test locally. The pinned versions are working well. I left a comment about the "orig" space. Also, in the call to init_anat_preproc_wf(), the output_spaces argument should be changed to spaces.

@oesteban
Copy link
Member Author

Seems like unfollowing and following from my repo did the trick (regarding to CircleCI).

@oesteban oesteban force-pushed the maint/overhaul-2 branch 2 times, most recently from a92edd3 to 5478303 Compare March 11, 2020 01:11
- [x] Revise the downstreaming of the new objects in niworkflows to
  handle spaces.
- [x] Add ``--fs-subjects-dir`` flag (see nipreps/fmriprep#1901)
@oesteban oesteban merged commit 7e4bce7 into nipreps:master Mar 11, 2020
@oesteban oesteban deleted the maint/overhaul-2 branch March 11, 2020 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants