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

[MRG] introduced fixed effects model #386

Open
wants to merge 3 commits into
base: master
from

Conversation

@bthirion
Copy link
Contributor

commented Sep 6, 2019

Addresses #378 #350 #281

bthirion added 2 commits Sep 6, 2019
@codecov

This comment has been minimized.

Copy link

commented Sep 8, 2019

Codecov Report

Merging #386 into master will increase coverage by 0.18%.
The diff coverage is 98.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #386      +/-   ##
==========================================
+ Coverage   91.08%   91.26%   +0.18%     
==========================================
  Files          34       34              
  Lines        3734     3790      +56     
==========================================
+ Hits         3401     3459      +58     
+ Misses        333      331       -2
Impacted Files Coverage Δ
nistats/tests/test_first_level_model.py 100% <100%> (ø) ⬆️
nistats/tests/test_contrasts.py 100% <100%> (ø) ⬆️
nistats/contrasts.py 93.28% <96.15%> (+0.6%) ⬆️
nistats/__init__.py 94.73% <0%> (+5.26%) ⬆️
nistats/tests/test_init.py 94.44% <0%> (+5.55%) ⬆️

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 bc0886b...143565b. Read the comment docs.

@bthirion

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

@kchawla-pi any hint about windows failure ?

@kchawla-pi

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

The still in memory objects must be manually deleted using thedel keyword before exiting the temporary directory context

@jeromedockes

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

@kchawla-pi any hint about windows failure ?

the mask NiftiImage holds a file handle to the .nii file on disk in its file_map. in windows that file cannot be removed while there is an open file handle

@bthirion

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

Could I get a review on this PR ? Neurostars is asking for it...

Example of explicit fixed effects fMRI model fitting
====================================================
This example illustrates how to

This comment has been minimized.

Copy link
@kchawla-pi

kchawla-pi Sep 11, 2019

Member

Incomplete text.

repetition. Hum Brain Mapp. 2006: 27:360--371.
http://www.pubmedcentral.nih.gov/articlerender.fcgi?artid=2653076#R11
Please see `plot_fiac_analysis.py` example for details. The main

This comment has been minimized.

Copy link
@kchawla-pi

kchawla-pi Sep 11, 2019

Member

This should be internally hyperlinked to the appropriate example.


###############################################################################
# Create a write directory to work
# it will be a 'results' subdirectory of the current directory.

This comment has been minimized.

Copy link
@kchawla-pi

kchawla-pi Sep 11, 2019

Member

This sentence needs to be rewritten.

# Prepare data and analysis parameters
# --------------------------------------
#
# Note that there are two sessions

This comment has been minimized.

Copy link
@kchawla-pi

kchawla-pi Sep 11, 2019

Member

We should mention where to note the two sessions and how we know the keys to use. Something like:
"Looking at the description we see the way to access the data is using the keys...etc"

cut_coords = [-129, -126, 49]

fmri_glm = fmri_glm.fit(fmri_img[0], design_matrices=design_matrices[0])
dic1 = fmri_glm.compute_contrast(contrast_val, output_type='all')

This comment has been minimized.

Copy link
@kchawla-pi

kchawla-pi Sep 11, 2019

Member

We need a better variable name dic1 doesn't tell the user anything about the contents of the variable.

#
p = 100
X1, V1 = np.random.randn(p), np.ones(p)
Xf, Vf, tf = _fixed_effects([X1, 2 * X1], [V1, 4 * V1],

This comment has been minimized.

Copy link
@kchawla-pi

kchawla-pi Sep 11, 2019

Member

Fuller variable names will be helpful for someone like me.

@@ -59,6 +59,60 @@ def test_high_level_glm_one_session():
assert_true(isinstance(z1, Nifti1Image))


def test_fixed_effects():

This comment has been minimized.

Copy link
@kchawla-pi

kchawla-pi Sep 11, 2019

Member

Rename this appropriately when the testee function is renamed.

@@ -59,6 +59,60 @@ def test_high_level_glm_one_session():
assert_true(isinstance(z1, Nifti1Image))


def test_fixed_effects():
# New API

This comment has been minimized.

Copy link
@kchawla-pi

kchawla-pi Sep 11, 2019

Member

What's this # New API comment I see everywhere? Without more details, it is confusing. How new is new? What's new about it?

contrasts,
ffx_contrast, ffx_variance, ffx_stat,
dic1, dic2, ffx_dic,
)

This comment has been minimized.

Copy link
@kchawla-pi

kchawla-pi Sep 11, 2019

Member

We should figure out which one is causing the problem and only delete that. It's probably the mask, since NiftiMasker/Nibabel doesn't release data handles easily.. Rest are in memory variables.

@@ -527,3 +581,4 @@ def test_param_mask_deprecation_first_level_models_from_bids():
for param_warning_ in raised_param_deprecation_warnings:
assert str(param_warning_.message) == deprecation_msg
assert param_warning_.category is DeprecationWarning

This comment has been minimized.

Copy link
@kchawla-pi

kchawla-pi Sep 11, 2019

Member

Empty line at the end is a flake8 violation.

@kchawla-pi

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Add a whats_new entry.

# For this, we first define the contrasts as we would do for a single session
n_columns = design_matrices[0].shape[1]

def pad_vector(contrast_, n_columns):

This comment has been minimized.

Copy link
@jeromedockes

jeromedockes Sep 15, 2019

Collaborator

this is used only once in the example, so maybe it can be inlined and avoid the function definition

This comment has been minimized.

Copy link
@kchawla-pi

kchawla-pi Sep 15, 2019

Member

I like using the function, I am wondering if we should include this as an automatic feature at some point in the future because many examples seem to need padding.

# Create a write directory to work
# it will be a 'results' subdirectory of the current directory.
from os import mkdir, path, getcwd
write_dir = path.join(getcwd(), 'results')

This comment has been minimized.

Copy link
@jeromedockes

jeromedockes Sep 15, 2019

Collaborator

where is this used?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.