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: Add number of volumes per *b*-value to QC report #129

Closed
wants to merge 1 commit into from
Closed

ENH: Add number of volumes per *b*-value to QC report #129

wants to merge 1 commit into from

Conversation

josephmje
Copy link
Collaborator

No description provided.

@oesteban oesteban changed the title WIP: Adds number of volumes per b-value to QC report ENH: Add number of volumes per *b*-value to QC report Dec 10, 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.

Nitpicks here and there

@@ -29,6 +29,7 @@ class _CheckGradientTableOutputSpec(TraitedSpec):
out_bvec = File(exists=True)
full_sphere = traits.Bool()
pole = traits.Tuple(traits.Float, traits.Float, traits.Float)
num_shells = traits.Dict()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
num_shells = traits.Dict()
num_shells = traits.Int
shells_dist = traits.Dict

@@ -81,6 +86,7 @@ def _run_interface(self, runtime):
pole = table.pole
self._results["pole"] = tuple(pole)
self._results["full_sphere"] = np.all(pole == 0.0)
self._results["num_shells"] = table.count_shells
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self._results["num_shells"] = table.count_shells
self._results["num_shells"] = len(table.count_shells)
self._results["shells_dist"] = table.count_shells

Comment on lines +62 to +63
>>> check.outputs.num_shells
{0: 12, 1200: 32, 2500: 61}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>>> check.outputs.num_shells
{0: 12, 1200: 32, 2500: 61}
>>> check.outputs.num_shells
3
>>> check.outputs.shells_dist
{0: 12, 1200: 32, 2500: 61}

Comment on lines +51 to +52
>>> check.outputs.num_shells
{0.0: 12, 1200.0: 32, 2500.0: 61}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>>> check.outputs.num_shells
{0.0: 12, 1200.0: 32, 2500.0: 61}
>>> check.outputs.num_shells
3
>>> check.outputs.shells_dist
{0.0: 12, 1200.0: 32, 2500.0: 61}

@oesteban oesteban marked this pull request as ready for review December 10, 2020 19:13
@oesteban
Copy link
Member

@josephmje do you think we can push this in early next week?

@josephmje
Copy link
Collaborator Author

@josephmje do you think we can push this in early next week?

Yes! Will do. I'll also add the part to show it in the reportlet. I may need to create a new PR as I accidentally got rid of this branch.

@josephmje
Copy link
Collaborator Author

Closing this in favour of #150 due to the old commits belonging to a deleted branch.

@josephmje josephmje closed this Jan 12, 2021
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.

2 participants