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

TST: Test for invariance of model_params to splitting of the data. #1113

Merged
merged 3 commits into from Oct 31, 2016

Conversation

Projects
None yet
4 participants
@arokem
Member

arokem commented Sep 1, 2016

This looks like a bug to me: it shouldn't matter whether you fit the
entire data array or just one.

This happens because min_signal currently depends on the entire data array
that is passed, and that varies here depending on whether only one voxel
or the entire array is passed.

For now, I only have the bug and no fix, hence the WIP. Suggestions most welcome!

@arokem arokem changed the title from TST: Test for invariance of model_params to splitting of the data. to WIP: TST: Test for invariance of model_params to splitting of the data. Sep 1, 2016

@arokem

This comment has been minimized.

Member

arokem commented Sep 1, 2016

How about this for a solution? Removing the WIP tag.

@arokem arokem changed the title from WIP: TST: Test for invariance of model_params to splitting of the data. to TST: Test for invariance of model_params to splitting of the data. Sep 1, 2016

@codecov-io

This comment has been minimized.

codecov-io commented Sep 2, 2016

Current coverage is 84.61% (diff: 100%)

Merging #1113 into master will increase coverage by <.01%

@@             master      #1113   diff @@
==========================================
  Files           219        219          
  Lines         24879      24884     +5   
  Methods           0          0          
  Messages          0          0          
  Branches       2515       2514     -1   
==========================================
+ Hits          21051      21056     +5   
  Misses         3199       3199          
  Partials        629        629          

Powered by Codecov. Last update e3b6b9e...69defe0

@coveralls

This comment has been minimized.

coveralls commented Sep 2, 2016

Coverage Status

Coverage increased (+0.004%) to 83.029% when pulling 9c15476 on arokem:min-signal-context into 31bc6fe on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 2, 2016

Coverage Status

Coverage increased (+0.004%) to 83.029% when pulling 9c15476 on arokem:min-signal-context into 31bc6fe on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Oct 21, 2016

Anyone want to give this one a review? It's a rather simple bug that really shouldn't be here, with a rather simple fix (I think) -- less than 40 lines of changes.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 21, 2016

LGTM. It makes sense to use a constant value for the min signal value if none was provided.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 31, 2016

Oh, there is now a conflict because I merged your other PR.

arokem added some commits Sep 1, 2016

TST: Test for invariance of model_params to splitting of the data.
This looks like a bug to me: it shouldn't matter whether you fit the
entire data array or just one.

This happens because min_signal currently depends on the entire data array
that is passed, and that varies here depending on whether only one voxel
or the entire array is passed.

@arokem arokem force-pushed the arokem:min-signal-context branch from 9c15476 to 69defe0 Oct 31, 2016

@arokem

This comment has been minimized.

Member

arokem commented Oct 31, 2016

Rebased!

It looks like all conflicts were in docstrings, but maybe let Travis run through this, just to be on the safe side.

@coveralls

This comment has been minimized.

coveralls commented Oct 31, 2016

Coverage Status

Coverage increased (+0.003%) to 87.144% when pulling 69defe0 on arokem:min-signal-context into e3b6b9e on nipy:master.

@MarcCote MarcCote merged commit d4f45f2 into nipy:master Oct 31, 2016

4 checks passed

codecov/patch 100% of diff hit (target 84.61%)
Details
codecov/project 84.61% (+<.01%) compared to e3b6b9e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.003%) to 87.144%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment