poldracklab / fitlins

Fit Linear Models to BIDS Datasets
https://fitlins.readthedocs.io
Apache License 2.0
76 stars 32 forks source link

FIX: Handle correct contrasts and intercepts being passed from PyBIDS #387

Closed effigies closed 2 years ago

codecov-commenter commented 2 years ago

Codecov Report

Base: 64.45% // Head: 64.18% // Decreases project coverage by -0.26% :warning:

Coverage data is based on head (7ee441f) compared to base (d0c0dee). Patch coverage: 44.44% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #387 +/- ## ========================================== - Coverage 64.45% 64.18% -0.27% ========================================== Files 23 23 Lines 1820 1829 +9 Branches 345 298 -47 ========================================== + Hits 1173 1174 +1 - Misses 565 570 +5 - Partials 82 85 +3 ``` | [Impacted Files](https://codecov.io/gh/poldracklab/fitlins/pull/387?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=poldracklab) | Coverage Δ | | |---|---|---| | [fitlins/cli/run.py](https://codecov.io/gh/poldracklab/fitlins/pull/387/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=poldracklab#diff-Zml0bGlucy9jbGkvcnVuLnB5) | `0.00% <0.00%> (ø)` | | | [fitlins/conftest.py](https://codecov.io/gh/poldracklab/fitlins/pull/387/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=poldracklab#diff-Zml0bGlucy9jb25mdGVzdC5weQ==) | `100.00% <ø> (ø)` | | | [fitlins/interfaces/visualizations.py](https://codecov.io/gh/poldracklab/fitlins/pull/387/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=poldracklab#diff-Zml0bGlucy9pbnRlcmZhY2VzL3Zpc3VhbGl6YXRpb25zLnB5) | `57.59% <ø> (-1.27%)` | :arrow_down: | | [fitlins/interfaces/afni.py](https://codecov.io/gh/poldracklab/fitlins/pull/387/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=poldracklab#diff-Zml0bGlucy9pbnRlcmZhY2VzL2FmbmkucHk=) | `86.13% <50.00%> (-0.58%)` | :arrow_down: | | [fitlins/interfaces/nistats.py](https://codecov.io/gh/poldracklab/fitlins/pull/387/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=poldracklab#diff-Zml0bGlucy9pbnRlcmZhY2VzL25pc3RhdHMucHk=) | `71.78% <75.00%> (+0.05%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=poldracklab). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=poldracklab)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

effigies commented 2 years ago

@adelavega @jmumford @jdkent Looks like 0.15.3 runs, but now we're getting different values in our results. Suggests we're now using different design matrices, as nilearn and AFNI aren't changing, and neither are the input masks.

Do any of you remember changes that we would have expected to improve design matrix construction?

jmumford commented 2 years ago

Could it be a change in the smoothing or highpass filtering options?

effigies commented 2 years ago

Did we change those in PyBIDS? The only difference between this and the failing tests on the dev branch is that this is using pybids 0.15.1 and not 0.15.3.

adelavega commented 2 years ago

Hmm. I guess I'd like to see a diff between the two DMs.

The only thing I can think of is that we were not modeling an intercept is some situations, and possibly we are adding an intercept where we shouldn't be?

Unless there is a major bug (i.e. dropped columns), not sure where else the DMs would differ

effigies commented 2 years ago

@adelavega Yes, the difference is that now there's an explicit intercept column. This didn't get overridden by nistats, which produced a constant column instead.

effigies commented 2 years ago

FWIW I did write up my method for reproducing, since I wanted to get it set up on another system. https://github.com/poldracklab/fitlins/wiki/Testing-FitLins-regressions-on-example-data

adelavega commented 2 years ago

Awesome, so that should be a fitlins issue then, correct?

effigies commented 2 years ago

Well, it fixes the regression test, so that's good. Might have revealed other bugs in FitLins, though...