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

FIX: Limit NaN imputation and use mean non-zero value #57

Merged
merged 12 commits into from
Aug 1, 2018

Conversation

effigies
Copy link
Collaborator

@effigies effigies commented Jul 27, 2018

Fixes #56.

@adelavega
Copy link
Collaborator

👌

@adelavega
Copy link
Collaborator

My PR is merged. Oh by the way, maybe it's not a bad idea to warn users (or throw an exception) if n/as are found in non-imputable variables?

@effigies
Copy link
Collaborator Author

effigies commented Jul 28, 2018

Actually, do we even want to by-default impute any but the expected n/as for the first volume? It seems to me that anything else should go through a transform or raise an error.

And to complicate matters further, non-steady-state volumes will push down the "first" volume.

@adelavega
Copy link
Collaborator

I agree. I just think a nice exception would be nice to have.

@effigies
Copy link
Collaborator Author

And to complicate matters further, non-steady-state volumes will push down the "first" volume.

Looks like fMRIPrep does not remove non-steady-state volumes before calculating FD/DVARS, so we don't actually need to account for that. (The nistats interface will need to zero them out, but we should already be doing that.)

          CSF  WhiteMatter  GlobalSignal   stdDVARS  non-stdDVARS  \
0  608.277697    84.968374    289.321942        NaN           NaN   
1  -36.020893    -1.145165     -8.573609  10.742792    365.722839   

   vx-wisestdDVARS  FramewiseDisplacement  tCompCor00  tCompCor01  tCompCor02  \
0              NaN                    NaN    0.000000    0.000000    0.000000   
1        11.746696               0.495028   -0.013686    0.028413    0.012123   

   ...   Cosine03  Cosine04  Cosine05  NonSteadyStateOutlier00         X  \
0  ...    0.00000  0.000000  0.000000                      1.0 -0.006812   
1  ...    0.10448  0.104445  0.104403                      0.0  0.000000   

         Y        Z      RotX  RotY  RotZ  
0  0.00000  0.00000  0.000000  -0.0   0.0  
1 -0.03263  0.37969  0.001518  -0.0   0.0  

[2 rows x 32 columns]

@effigies
Copy link
Collaborator Author

Okay. Modulo some prettifying CSS, this is working for me. @adelavega do you want to give it a shot?

@effigies effigies merged commit eabadee into poldracklab:master Aug 1, 2018
@effigies effigies deleted the fix/fillna branch August 1, 2018 15:03
@adelavega
Copy link
Collaborator

can you do a release/push to docker hub? it'll make it easier for me to test

@effigies
Copy link
Collaborator Author

effigies commented Aug 1, 2018

So how does it actually get run for you? Do you call docker from neuroscout?

@adelavega
Copy link
Collaborator

I extend your build with a Dockerfile: https://github.com/neuroscout/neuroscout-cli/blob/master/Dockerfile

(just realized I may be re-installing fitlins unnecessarily)

@effigies
Copy link
Collaborator Author

effigies commented Aug 1, 2018

0.0.5 is on Docker Hub.

@adelavega
Copy link
Collaborator

Cheers

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.

None yet

2 participants