Skip to content

Conversation

dPys
Copy link
Collaborator

@dPys dPys commented Feb 11, 2020

*All of these support the signal prediction routines.
*Each function will need a unit test.

@pull-assistant
Copy link

pull-assistant bot commented Feb 11, 2020

@dPys dPys changed the title [ENH] Miscellaneous emc helper functions vectors [ENH] Miscellaneous emc helper functions -- vectors Feb 11, 2020
@dPys dPys changed the title [ENH] Miscellaneous emc helper functions -- vectors [ENH] Add emc helper functions - vectors Feb 11, 2020
@dPys dPys requested a review from oesteban February 12, 2020 02:48
arokem added a commit to arokem/dmriprep that referenced this pull request Feb 25, 2020
Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

Unit tests for _nonoverlapping_qspace_samples would be great to have and it is a very low hanging fruit - we want this function to be super-robust.

Comment on lines 404 to 432
def _rasb_to_bvec_list(in_rasb):
"""
Create a list of b-vectors from a rasb gradient table.
Parameters
----------
in_rasb : str or os.pathlike
File path to a RAS-B gradient table.
"""
import numpy as np

ras_b_mat = np.genfromtxt(in_rasb, delimiter="\t")
bvec = [vec for vec in ras_b_mat[:, 0:3] if not np.isclose(all(vec), 0)]
return list(bvec)


def _rasb_to_bval_floats(in_rasb):
"""
Create a list of b-values from a rasb gradient table.
Parameters
----------
in_rasb : str or os.pathlike
File path to a RAS-B gradient table.
"""
import numpy as np

ras_b_mat = np.genfromtxt(in_rasb, delimiter="\t")
return [float(bval) for bval in ras_b_mat[:, 3] if bval > 0]
Copy link
Member

Choose a reason for hiding this comment

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

Can you point me at the location where these are needed? I believe our Gradients class has already this covered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(and not covered in Gradients class)

@oesteban oesteban added this to the 0.4.0 milestone Mar 23, 2020
@pep8speaks
Copy link

pep8speaks commented Mar 24, 2020

Hello @dPys, 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-24 20:09:36 UTC

@codecov
Copy link

codecov bot commented Mar 24, 2020

Codecov Report

❌ Patch coverage is 62.33766% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.42%. Comparing base (c695f4c) to head (8b365f8).
⚠️ Report is 392 commits behind head on master.

Files with missing lines Patch % Lines
dmriprep/utils/registration.py 35.55% 29 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
- Coverage   52.37%   50.42%   -1.95%     
==========================================
  Files          16       22       +6     
  Lines         907     1285     +378     
  Branches      114      168      +54     
==========================================
+ Hits          475      648     +173     
- Misses        431      622     +191     
- Partials        1       15      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dPys dPys force-pushed the miscellaneous_emc_helper_functions_vectors branch from 49e0a0a to 7e1b11e Compare March 24, 2020 18:12
@dPys dPys mentioned this pull request Mar 25, 2020
@oesteban oesteban modified the milestones: 0.4.0, 0.5.0 Dec 10, 2020
@oesteban
Copy link
Member

I'm going to clean up the repo. However, I still believe this function would perfectly be part of DIPY or a smaller nipreps package for diffusion MRI (I'm thinking of calling it nidiff)or something of the sort. It could be useful for MRIQC and others, and dmriprep is too voluminous to add it only for this feature.

When that happens, I'll make sure to annotate @dPys as the co-author in all commits.

@oesteban oesteban closed this Oct 15, 2025
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.

4 participants