Skip to content

ENH: Do not raise error in all instances of b-vecs/vals inconsistencies #100

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

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Apr 6, 2020

We raised a ValueError whenever the number of b-vals and all-zero b-vecs
didn't match. Few lines later, we would overwrite nonzero b-vecs to
match the b-vals, but that step was never reached. This PR allows the
function to just throw a warning instead.

Resolves: #82

We raised a ValueError whenever the number of b-vals and all-zero b-vecs
didn't match. Few lines later, we would overwrite nonzero b-vecs to
match the b-vals, but that step was never reached. This PR allows the
function to just throw a warning instead.

Resolves: nipreps#82
@pull-assistant
Copy link

pull-assistant bot commented Apr 6, 2020

Score: 1.00

Best reviewed: commit by commit


Optimal code review plan

     ENH: Do not raise error in all instances of b-vecs/vals inconsistencie...

Powered by Pull Assistant. Last update d1b93aa ... d1b93aa. Read the comment docs.

@oesteban
Copy link
Member Author

oesteban commented Apr 6, 2020

This is working locally. Although I'm not super-satisfied with the logger I've assigned this warning, I think it will do for now.

@oesteban oesteban requested a review from arokem April 6, 2020 22:49
@codecov
Copy link

codecov bot commented Apr 6, 2020

Codecov Report

Merging #100 into master will decrease coverage by 0.82%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
- Coverage   51.64%   50.81%   -0.83%     
==========================================
  Files          21       21              
  Lines        1218     1222       +4     
  Branches      161      162       +1     
==========================================
- Hits          629      621       -8     
- Misses        578      586       +8     
- Partials       11       15       +4     
Impacted Files Coverage Δ
dmriprep/utils/vectors.py 90.24% <60.00%> (-4.14%) ⬇️
dmriprep/workflows/dwi/base.py 30.00% <0.00%> (-16.67%) ⬇️

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 2802c50...d1b93aa. Read the comment docs.

@oesteban
Copy link
Member Author

oesteban commented Apr 7, 2020

Okay, at the risk of merging some suboptimal code I'll go ahead so that we have more stuff to revise at tomorrow's meeting (also I'm feeling some urgency of fixing master for this new dataset so that I can continue with #97).

@oesteban oesteban merged commit a27fc16 into nipreps:master Apr 7, 2020
@oesteban oesteban deleted the fix/82-inconsistent-b-vals-vecs branch April 7, 2020 01:51
oesteban added a commit to oesteban/dmriprep that referenced this pull request Apr 13, 2020
This PR completes a previous one regarding the diffusion table.

Amends: nipreps#100
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.

Inconsistent bvals and bvecs
1 participant