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

Documentation to discourage misuse of GradientTable #1059

Merged
merged 4 commits into from Jul 15, 2016

Conversation

Projects
None yet
5 participants
@kesshijordan
Contributor

kesshijordan commented May 25, 2016

This was misused in the nipype implementation such that the bvals were being set as attributes after the creation of the GradientTable object (so the tensor fit didn't use the bvalues). mrbago suggested I add to the documentation to prevent this in the future.

@arokem

This comment has been minimized.

Member

arokem commented May 25, 2016

Yeah - I saw your PR over on nipy/nipype#1484. Do you think that clarification of the inputs would be sufficient? That is, instead of what we currently have, rewrite the documentation of the gradients input to be:

gradients : array_like (N, 3)
     Diffusion gradients. The direction of each of these vectors corresponds to the b-vector, and the 
     length corresponds to the b-value.

Or would people still be likely to miss the point?

@MrBago

This comment has been minimized.

Contributor

MrBago commented May 26, 2016

@arokem Do you think a doc fix is enough here or do you think it's worth trying to "lock down" the instance a little bit? In principle this applies to any class that uses the @atuo_attr decorator right? I vote for doc fix, but I think it's worth asking the question.

@arokem

This comment has been minimized.

Member

arokem commented May 27, 2016

I think that a doc-fix should be enough.

@arokem

This comment has been minimized.

Member

arokem commented Jul 1, 2016

Hello @kesshijordan : any more thoughts here?

@kesshijordan

This comment has been minimized.

Contributor

kesshijordan commented Jul 3, 2016

I think a doc fix is good.

On Jul 1, 2016, at 7:36 AM, Ariel Rokem <notifications@github.commailto:notifications@github.com> wrote:

Hello @kesshijordanhttps://github.com/kesshijordan : any more thoughts here?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/1059#issuecomment-229963619, or mute the threadhttps://github.com/notifications/unsubscribe/AD2x8zDOx1HneydOBM6jcmkVlNmJXNJIks5qRSYGgaJpZM4Im8bY.

@arokem

This comment has been minimized.

Member

arokem commented Jul 4, 2016

Thanks. And do you think that it would be enough to document the gradients parameter (as I suggested here: #1059 (comment)), or do you think we should add a note (as you are proposing here)? Either way, a "note" section should follow after the "parameters" and "returns", not be added in front (as you propose; see numpy docstring standards for details: https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt).

@kesshijordan

This comment has been minimized.

Contributor

kesshijordan commented Jul 5, 2016

I think it's nice to include both, since we aren't "locking down" the instance... the misuse won't, necessarily, return an error that is easily noticed... the user has to know what scale the output image values should be on in order to catch it. Re formatting: I'll rearrange and commit.

@kesshijordan

This comment has been minimized.

Contributor

kesshijordan commented Jul 5, 2016

How does that look?

@coveralls

This comment has been minimized.

coveralls commented Jul 5, 2016

Coverage Status

Coverage remained the same at 82.908% when pulling 2c4934f on kesshijordan:master into fbad873 on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Jul 5, 2016

👍 from me!

Let's give others a chance to chime in, but if I don't hear anything I'll merge this in a few days.

@codecov-io

This comment has been minimized.

codecov-io commented Jul 5, 2016

Current coverage is 80.84%

Merging #1059 into master will not change coverage

@@             master      #1059   diff @@
==========================================
  Files           200        200          
  Lines         23023      23023          
  Methods           0          0          
  Messages          0          0          
  Branches       2309       2309          
==========================================
  Hits          18614      18614          
  Misses         3933       3933          
  Partials        476        476          

Powered by Codecov. Last updated by fbad873...2c4934f

@arokem

This comment has been minimized.

Member

arokem commented Jul 15, 2016

Thanks!

@arokem arokem merged commit 64ed68d into nipy:master Jul 15, 2016

4 checks passed

codecov/patch Coverage not affected when comparing fbad873...2c4934f
Details
codecov/project 80.84% (+0.00%) compared to fbad873
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 82.908%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment