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

ENH: Update image utility output path behaviour #81

Merged
merged 9 commits into from
Mar 20, 2020
Merged

ENH: Update image utility output path behaviour #81

merged 9 commits into from
Mar 20, 2020

Conversation

josephmje
Copy link
Collaborator

No description provided.

@pull-assistant
Copy link

pull-assistant bot commented Feb 25, 2020

Score: 0.84

Best reviewed: commit by commit


Optimal code review plan (1 warning)

update out_path

dmriprep/interfaces/images.py 50% changes removed in fix image interfaces

     Update dmriprep/utils/images.py

     Update dmriprep/utils/images.py

     Update dmriprep/utils/images.py

     Apply @oesteban's suggestions

     fix pep8

     Update dmriprep/utils/images.py

     modify newpath to out_path

     fix image interfaces

Powered by Pull Assistant. Last update 6877fa1 ... a0e205c. Read the comment docs.

dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 25, 2020

Codecov Report

Merging #81 into master will decrease coverage by 0.3%.
The diff coverage is 17.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
- Coverage   51.85%   51.54%   -0.31%     
==========================================
  Files          20       21       +1     
  Lines        1186     1197      +11     
  Branches      151      155       +4     
==========================================
+ Hits          615      617       +2     
- Misses        564      573       +9     
  Partials        7        7
Impacted Files Coverage Δ
dmriprep/utils/images.py 13.33% <13.33%> (ø)
dmriprep/interfaces/images.py 70.58% <36.36%> (+29.41%) ⬆️

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 199a496...a0e205c. Read the comment docs.

@josephmje
Copy link
Collaborator Author

@oesteban should dtype be added as an argument to the other functions as well?

dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Feb 25, 2020

Hello @josephmje, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 dmriprep.

Comment last updated at 2020-03-17 19:25:53 UTC

@ghost
Copy link

ghost commented Feb 27, 2020

Warnings are found on analyzing the commit e962756.

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. 💬

@josephmje
Copy link
Collaborator Author

We might need to change the version of nipype that's pinned. The new version is getting TraitType from traits.api but the older one imported from traits.trait_handlers.

@oesteban
Copy link
Member

Could you rebase?

josephmje and others added 9 commits March 16, 2020 11:34
Co-Authored-By: Oscar Esteban <code@oscaresteban.es>
Co-Authored-By: Oscar Esteban <code@oscaresteban.es>
Co-Authored-By: Oscar Esteban <code@oscaresteban.es>
Co-Authored-By: Oscar Esteban <code@oscaresteban.es>
Co-Authored-By: Oscar Esteban <code@oscaresteban.es>
@oesteban oesteban merged commit 23d8db4 into nipreps:master Mar 20, 2020
self._results['out_b0s'] = rescale_b0(
self.inputs.in_file,
self.inputs.mask_file,
newpath=runtime.cwd
self.inputs.mask_file, out_b0s
Copy link
Member

Choose a reason for hiding this comment

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

I think this is what introduced #90 - the mask and the b0s are swapped.

Copy link
Collaborator Author

@josephmje josephmje Apr 6, 2020

Choose a reason for hiding this comment

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

it failed at the very first step where the b0s get extracted. the difference between the 2 dwi images for that subject is that one contains multiple b0s and the borked contains a single b0. it looks like this single b0 case is messing things up.

Copy link
Member

Choose a reason for hiding this comment

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

yup I realized after commenting. I'm still going through this - I might have found a solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants