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

NF Add the parameter fa_operator in auto_response function #1288

Merged
merged 6 commits into from Jul 5, 2017

Conversation

Projects
None yet
8 participants
@GuillaumeTh
Contributor

GuillaumeTh commented Jun 29, 2017

Add a new parameter (fa_operator) in auto_response to give an operator for the comparison with the FA threshold.

@jchoude, @arokem, @MrBago Can you review ? This PR will be useful when the MSMT PR will be merged.

@arokem

This comment has been minimized.

Member

arokem commented Jun 29, 2017

Looks useful. I'd prefer to have the operator be a proper callable (defined as a function), rather than a lambda defined in the function signature (that looks a bit weird to me). I'd also want to see a test that defines this callable as something different, to get a sense for how this will be used.

operator used to compare the FA with the fa_thr.
fa_operator : function
Function that defines operator used to compare the FA with the fa_thr.
Must be defined as `def function_name(FA, fa_thr)` and return a bool array.

This comment has been minimized.

@MrBago

MrBago Jun 29, 2017

Contributor

why? I believe lambdas and functions are the same in python (expect lambdas don't get a function name).

This comment has been minimized.

@GuillaumeTh

GuillaumeTh Jun 30, 2017

Contributor

So, I don't know if my last commit is the right thing or not

This comment has been minimized.

@arokem

arokem Jun 30, 2017

Member

I think that the code is fine (I think it's easier to understand this way). The documentation can be changed to say:

fa_operator : callable
    A callable that defines an operation that compares FA with the fa_thr. The operator 
    should have two positional arguments (e.g., `fa_operator(FA, fa_thr)`) and it should
    return a bool array.
@coveralls

This comment has been minimized.

coveralls commented Jun 29, 2017

Coverage Status

Coverage increased (+0.01%) to 85.429% when pulling 8e12775 on GuillaumeTh:NF_auto_response into 0976d9e on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 29, 2017

Coverage Status

Coverage increased (+0.01%) to 85.429% when pulling 8e12775 on GuillaumeTh:NF_auto_response into 0976d9e on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Jun 29, 2017

Codecov Report

Merging #1288 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1288      +/-   ##
==========================================
+ Coverage   87.11%   87.12%   +0.01%     
==========================================
  Files         228      228              
  Lines       28764    28791      +27     
  Branches     3090     3093       +3     
==========================================
+ Hits        25057    25084      +27     
  Misses       3003     3003              
  Partials      704      704
Impacted Files Coverage Δ
dipy/reconst/csdeconv.py 88.67% <100%> (+0.14%) ⬆️
dipy/reconst/tests/test_csdeconv.py 99.42% <100%> (+0.03%) ⬆️
dipy/core/gradients.py 97.36% <0%> (+0.04%) ⬆️

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 0976d9e...1120d20. Read the comment docs.

@coveralls

This comment has been minimized.

coveralls commented Jun 29, 2017

Coverage Status

Coverage increased (+0.01%) to 85.429% when pulling 8e12775 on GuillaumeTh:NF_auto_response into 0976d9e on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 29, 2017

Coverage Status

Coverage increased (+0.01%) to 85.429% when pulling 8e12775 on GuillaumeTh:NF_auto_response into 0976d9e on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 1, 2017

Coverage Status

Coverage increased (+0.01%) to 85.429% when pulling cef71ac on GuillaumeTh:NF_auto_response into 0976d9e on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jul 1, 2017

Coverage Status

Coverage increased (+0.01%) to 85.429% when pulling cef71ac on GuillaumeTh:NF_auto_response into 0976d9e on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Jul 1, 2017

This looks good to me. Unless I hear any objections, I will merge this after the holiday (July 4th).

@skoudoro

I just request a small change, otherwise, it looks good to me too.

@arokem , I think it will be better to wait #1283 to be merged before merging this one.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 3, 2017

I am not sure about the name. fa_operator? fa_callable is preferred.

@jchoude

This comment has been minimized.

Contributor

jchoude commented Jul 4, 2017

@skoudoro I work with @GuillaumeTh on this. I might be lost, but I don't see the change you requested. Mind pointing it?

Also, you say you prefer to merge #1283 before this one. Is it because both of them touch the test_csdeconv file? I don't think this might create any conflict.

No problem, I agree with you @jchoude and @arokem, you can merge this one, I will make this change later

@coveralls

This comment has been minimized.

coveralls commented Jul 4, 2017

Coverage Status

Coverage increased (+0.01%) to 85.43% when pulling 1120d20 on GuillaumeTh:NF_auto_response into 0976d9e on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jul 4, 2017

Coverage Status

Coverage increased (+0.01%) to 85.43% when pulling 1120d20 on GuillaumeTh:NF_auto_response into 0976d9e on nipy:master.

@Garyfallidis Garyfallidis merged commit 8fe7322 into nipy:master Jul 5, 2017

4 checks passed

codecov/patch 100% of diff hit (target 87.11%)
Details
codecov/project 87.12% (+0.01%) compared to 0976d9e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 85.43%
Details

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1288 from GuillaumeTh/NF_auto_response
NF Add the parameter fa_operator in auto_response function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment