ENH: Adopt row-major gradient tables #325
Merged
+164
−96
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #325 +/- ##
==========================================
- Coverage 77.01% 76.27% -0.74%
==========================================
Files 28 28
Lines 1775 1821 +46
Branches 177 193 +16
==========================================
+ Hits 1367 1389 +22
- Misses 348 361 +13
- Partials 60 71 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Author
|
In reviewing this codex-generated PR, I've remembered why we chose the fsl convention (column major): to make it consistent with the NIfTI order of axes (i.e., orientation indexed by the last axis). Not that we need to keep the fsl convention, but good to mention. cc/ @jhlegarreta @arokem |
Contributor
|
I still think that we should keep the row major convention, because
modeling that uses the gradients (e.g., DTI) more naturally thinks of this
as a matrix of n_directions by 4.
…On Thu, Nov 13, 2025 at 10:04 AM Oscar Esteban ***@***.***> wrote:
*oesteban* left a comment (nipreps/nifreeze#325)
<#325 (comment)>
In reviewing this codex-generated PR, I've remembered why we chose the fsl
convention (column major): to make it consistent with the NIfTI order of
axes (i.e., orientation indexed by the last axis). Not that we need to keep
the fsl convention, but good to mention. cc/ @jhlegarreta
<https://github.com/jhlegarreta> @arokem <https://github.com/arokem>
—
Reply to this email directly, view it on GitHub
<#325 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA46NSEF437Q3CBO3BYSND34TB35AVCNFSM6AAAAACMATKUKCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKMRZGA2TONBYG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
oesteban
commented
Nov 13, 2025
| raise ValueError("Gradient table must be a 2D array") | ||
|
|
||
| n_volumes = None | ||
| if self.dataobj is not None: |
Member
Author
There was a problem hiding this comment.
@jhlegarreta I think we disallowed this right (dataobj to be None)?
Contributor
oesteban
commented
Nov 13, 2025
jhlegarreta
approved these changes
Nov 13, 2025
Contributor
jhlegarreta
left a comment
jhlegarreta
left a comment
There was a problem hiding this comment.
In reviewing this codex-generated PR, I've remembered why we chose the fsl convention (column major): to make it consistent with the NIfTI order of axes (i.e., orientation indexed by the last axis). Not that we need to keep the fsl convention, but good to mention. cc/ @jhlegarreta @arokem
👍 Makes sense.
I still think that we should keep the row major convention, because
modeling that uses the gradients (e.g., DTI) more naturally thinks of this
as a matrix of n_directions by 4.
I am fine with whatever convention is most convenient internally; have not revisited since some time how GPR likes things vs how DIPY likes them.
This PR seems to do a good job documenting the dimensionality expected and checks pass. If we revisit this in the future, hopefully we will be in a better position to weigh in advantages/drawbacks or things that do not work smoothly and change the internal convention if necessary.
I have left a minor suggestion: we only have a pre-release, so we are not forced to respect the API or to refer to previous versions.
Co-authored-by: Jon Haitz Legarreta Gorroño <[email protected]>
oesteban
deleted the
codex/analyze-and-report-gradient-data-update-points
branch
jhlegarreta
added a commit
to jhlegarreta/nifreeze
that referenced
this pull request
Nov 17, 2025
Prefer colon over ellipsis to index remaining gradient dimension: gradients can only be two-dimensional arrays. Cross-reference nipreps#325.
jhlegarreta
added a commit
to jhlegarreta/nifreeze
that referenced
this pull request
Nov 17, 2025
Refactor gradient data checks in DWI data class: PR nipreps#325 introduced the row-major convention for gradient data in NiFreeze. However, gradient loading was not tested thoroughly. This resulted in some execution flows that would not guarantee a row-major internal convention or would crash under some circumstances. This commit refactors the gradient data checks and adds thorough testing: - Define all error or warning messages as global variables so that they can be checked exactly in tests. - Ensure that gradients conform to the row-major convention when instantiating the DWI class directly. This allows to separate the gradient reformatting from the dimensionality check with the DWI volume sequence. This simplifies the flow, as the gradient reformatting to the row-major convention does not depend on the number of volumes in the DWI sequence. Also, this makes the flow more consistent with the refactored checks of the NIfTI file-based loading utility function (`from_nii`). - Ensure that the gradients conform to the row-major convention immediately after loading the gradients file in the NIfTI file-based loading utility function (`from_nii`). As opposed to the previous implementation, this allows to load the gradients from a file where data follows either column-major or row-major convention. e.g. In the previous implementation the `if grad.shape[1] < 2:` was making an assumption about the layout and/or one that was wrong because we require 4 columns (direction (x,y,z) + b-value) or rows (if in column-major). The new implementation simplifies the execution flow.
jhlegarreta
added a commit
to jhlegarreta/nifreeze
that referenced
this pull request
Nov 17, 2025
Refactor gradient data checks in DWI data class: PR nipreps#325 introduced the row-major convention for gradient data in NiFreeze. However, gradient loading was not tested thoroughly. This resulted in some execution flows that would not guarantee a row-major internal convention or would crash under some circumstances. This commit refactors the gradient data checks and adds thorough testing: - Define all error or warning messages as global variables so that they can be checked exactly in tests. - Ensure that gradients conform to the row-major convention when instantiating the DWI class directly. This allows to separate the gradient reformatting from the dimensionality check with the DWI volume sequence. This simplifies the flow, as the gradient reformatting to the row-major convention does not depend on the number of volumes in the DWI sequence. Also, this makes the flow more consistent with the refactored checks of the NIfTI file-based loading utility function (`from_nii`). - Ensure that the gradients conform to the row-major convention immediately after loading the gradients file in the NIfTI file-based loading utility function (`from_nii`). As opposed to the previous implementation, this allows to load the gradients from a file where data follows either column-major or row-major convention. e.g. In the previous implementation the `if grad.shape[1] < 2:` was making an assumption about the layout and/or one that was wrong because we require 4 columns (direction (x,y,z) + b-value) or rows (if in column-major). The new implementation simplifies the execution flow.
jhlegarreta
added a commit
to jhlegarreta/nifreeze
that referenced
this pull request
Nov 17, 2025
Refactor gradient data checks in DWI data class: PR nipreps#325 introduced the row-major convention for gradient data in NiFreeze. However, gradient loading was not tested thoroughly. This resulted in some execution flows that would not guarantee a row-major internal convention or would crash under some circumstances. This commit refactors the gradient data checks and adds thorough testing: - Define all error or warning messages as global variables so that they can be checked exactly in tests. - Ensure that gradients conform to the row-major convention when instantiating the DWI class directly. This allows to separate the gradient reformatting from the dimensionality check with the DWI volume sequence. This simplifies the flow, as the gradient reformatting to the row-major convention does not depend on the number of volumes in the DWI sequence. Also, this makes the flow more consistent with the refactored checks of the NIfTI file-based loading utility function (`from_nii`). - Ensure that the gradients conform to the row-major convention immediately after loading the gradients file in the NIfTI file-based loading utility function (`from_nii`). As opposed to the previous implementation, this allows to load the gradients from a file where data follows either column-major or row-major convention. e.g. In the previous implementation the `if grad.shape[1] < 2:` was making an assumption about the layout and/or one that was wrong because we require 4 columns (direction (x,y,z) + b-value) or rows (if in column-major). The new implementation simplifies the execution flow.
jhlegarreta
added a commit
to jhlegarreta/nifreeze
that referenced
this pull request
Nov 17, 2025
Refactor gradient data checks in DWI data class: PR nipreps#325 introduced the row-major convention for gradient data in NiFreeze. However, gradient loading was not tested thoroughly. This resulted in some execution flows that would not guarantee a row-major internal convention or would crash under some circumstances. This commit refactors the gradient data checks and adds thorough testing: - Define all error or warning messages as global variables so that they can be checked exactly in tests. - Ensure that gradients conform to the row-major convention when instantiating the DWI class directly. This allows to separate the gradient reformatting from the dimensionality check with the DWI volume sequence. This simplifies the flow, as the gradient reformatting to the row-major convention does not depend on the number of volumes in the DWI sequence. Also, this makes the flow more consistent with the refactored checks of the NIfTI file-based loading utility function (`from_nii`). - Ensure that the gradients conform to the row-major convention immediately after loading the gradients file in the NIfTI file-based loading utility function (`from_nii`). As opposed to the previous implementation, this allows to load the gradients from a file where data follows either column-major or row-major convention. e.g. In the previous implementation the `if grad.shape[1] < 2:` was making an assumption about the layout and/or one that was wrong because we require 4 columns (direction (x,y,z) + b-value) or rows (if in column-major). The new implementation simplifies the execution flow.
jhlegarreta
added a commit
to jhlegarreta/nifreeze
that referenced
this pull request
Nov 20, 2025
Refactor gradient data checks in DWI data class: PR nipreps#325 introduced the row-major convention for gradient data in NiFreeze. However, gradient loading was not tested thoroughly. This resulted in some execution flows that would not guarantee a row-major internal convention or would crash under some circumstances. This commit refactors the gradient data checks and adds thorough testing: - Define all error or warning messages as global variables so that they can be checked exactly in tests. - Ensure that gradients conform to the row-major convention when instantiating the DWI class directly. This allows to separate the gradient reformatting from the dimensionality check with the DWI volume sequence. This simplifies the flow, as the gradient reformatting to the row-major convention does not depend on the number of volumes in the DWI sequence. Also, this makes the flow more consistent with the refactored checks of the NIfTI file-based loading utility function (`from_nii`). - Ensure that the gradients conform to the row-major convention immediately after loading the gradients file in the NIfTI file-based loading utility function (`from_nii`). As opposed to the previous implementation, this allows to load the gradients from a file where data follows either column-major or row-major convention. e.g. In the previous implementation the `if grad.shape[1] < 2:` was making an assumption about the layout and/or one that was wrong because we require 4 columns (direction (x,y,z) + b-value) or rows (if in column-major). The new implementation simplifies the execution flow.
jhlegarreta
added a commit
to jhlegarreta/nifreeze
that referenced
this pull request
Nov 21, 2025
Refactor gradient data checks in DWI data class: PR nipreps#325 introduced the row-major convention for gradient data in NiFreeze. However, gradient loading was not tested thoroughly. This resulted in some execution flows that would not guarantee a row-major internal convention or would crash under some circumstances. This commit refactors the gradient data checks and adds thorough testing: - Define all error or warning messages as global variables so that they can be checked exactly in tests. - Ensure that gradients conform to the row-major convention when instantiating the DWI class directly. This allows to separate the gradient reformatting from the dimensionality check with the DWI volume sequence. This simplifies the flow, as the gradient reformatting to the row-major convention does not depend on the number of volumes in the DWI sequence. Also, this makes the flow more consistent with the refactored checks of the NIfTI file-based loading utility function (`from_nii`). - Ensure that the gradients conform to the row-major convention immediately after loading the gradients file in the NIfTI file-based loading utility function (`from_nii`). As opposed to the previous implementation, this allows to load the gradients from a file where data follows either column-major or row-major convention. e.g. In the previous implementation the `if grad.shape[1] < 2:` was making an assumption about the layout and/or one that was wrong because we require 4 columns (direction (x,y,z) + b-value) or rows (if in column-major). The new implementation simplifies the execution flow.
jhlegarreta
added a commit
to jhlegarreta/nifreeze
that referenced
this pull request
Nov 21, 2025
Refactor gradient data checks in DWI data class: PR nipreps#325 introduced the row-major convention for gradient data in NiFreeze. However, gradient loading was not tested thoroughly. This resulted in some execution flows that would not guarantee a row-major internal convention or would crash under some circumstances. This commit refactors the gradient data checks and adds thorough testing: - Define all error or warning messages as global variables so that they can be checked exactly in tests. - Ensure that gradients conform to the row-major convention when instantiating the DWI class directly. This allows to separate the gradient reformatting from the dimensionality check with the DWI volume sequence. This simplifies the flow, as the gradient reformatting to the row-major convention does not depend on the number of volumes in the DWI sequence. Also, this makes the flow more consistent with the refactored checks of the NIfTI file-based loading utility function (`from_nii`). - Ensure that the gradients conform to the row-major convention immediately after loading the gradients file in the NIfTI file-based loading utility function (`from_nii`). As opposed to the previous implementation, this allows to load the gradients from a file where data follows either column-major or row-major convention. e.g. In the previous implementation the `if grad.shape[1] < 2:` was making an assumption about the layout and/or one that was wrong because we require 4 columns (direction (x,y,z) + b-value) or rows (if in column-major). The new implementation simplifies the execution flow.
jhlegarreta
added a commit
to jhlegarreta/nifreeze
that referenced
this pull request
Nov 22, 2025
Refactor gradient data checks in DWI data class: PR nipreps#325 introduced the row-major convention for gradient data in NiFreeze. However, gradient loading was not tested thoroughly. This resulted in some execution flows that would not guarantee a row-major internal convention or would crash under some circumstances. This commit refactors the gradient data checks and adds thorough testing: - Define all error or warning messages as global variables so that they can be checked exactly in tests. - Ensure that gradients conform to the row-major convention when instantiating the DWI class directly. This allows to separate the gradient reformatting from the dimensionality check with the DWI volume sequence. This simplifies the flow, as the gradient reformatting to the row-major convention does not depend on the number of volumes in the DWI sequence. Also, this makes the flow more consistent with the refactored checks of the NIfTI file-based loading utility function (`from_nii`). - Ensure that the gradients conform to the row-major convention immediately after loading the gradients file in the NIfTI file-based loading utility function (`from_nii`). As opposed to the previous implementation, this allows to load the gradients from a file where data follows either column-major or row-major convention. e.g. In the previous implementation the `if grad.shape[1] < 2:` was making an assumption about the layout and/or one that was wrong because we require 4 columns (direction (x,y,z) + b-value) or rows (if in column-major). The new implementation simplifies the execution flow.
jhlegarreta
added a commit
to jhlegarreta/nifreeze
that referenced
this pull request
Nov 22, 2025
Refactor gradient data checks in DWI data class: PR nipreps#325 introduced the row-major convention for gradient data in NiFreeze. However, gradient loading was not tested thoroughly. This resulted in some execution flows that would not guarantee a row-major internal convention or would crash under some circumstances. This commit refactors the gradient data checks and adds thorough testing: - Define all error or warning messages as global variables so that they can be checked exactly in tests. - Ensure that gradients conform to the row-major convention when instantiating the DWI class directly. This allows to separate the gradient reformatting from the dimensionality check with the DWI volume sequence. This simplifies the flow, as the gradient reformatting to the row-major convention does not depend on the number of volumes in the DWI sequence. Also, this makes the flow more consistent with the refactored checks of the NIfTI file-based loading utility function (`from_nii`). - Ensure that the gradients conform to the row-major convention immediately after loading the gradients file in the NIfTI file-based loading utility function (`from_nii`). As opposed to the previous implementation, this allows to load the gradients from a file where data follows either column-major or row-major convention. e.g. In the previous implementation the `if grad.shape[1] < 2:` was making an assumption about the layout and/or one that was wrong because we require 4 columns (direction (x,y,z) + b-value) or rows (if in column-major). The new implementation simplifies the execution flow.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Codex Task
Fixes #316.