ME-ICA / aroma

ICA-AROMA, as a Python package. A work in progress.
Apache License 2.0
7 stars 11 forks source link

Fix utils.py classification function errors #33

Closed vinferrer closed 4 years ago

vinferrer commented 4 years ago

Closes Nothing.

Changes proposed in this pull request:

vinferrer commented 4 years ago

Guys i runned pytest and this"feature_scores.txt file isn't been created here is the error:

        # Make sure files are generated
        assert isfile(join(out_path, "classification_overview.txt"))
        assert isfile(join(out_path, "classified_motion_ICs.txt"))
        assert isfile(join(out_path, "denoised_func_data_nonaggr.nii.gz"))
>       assert isfile(join(out_path, "feature_scores.txt"))
E       AssertionError: assert False
E        +  where False = isfile('/home/vicente/nilearn_data/development_fmri/development_fmri/out/feature_scores.txt')
E        +    where '/home/vicente/nilearn_data/development_fmri/development_fmri/out/feature_scores.txt' = join('/home/vicente/nilearn_data/development_fmri/development_fmri/out', 'feature_scores.txt')

test_integration.py:51: AssertionError
-------------------------------------------------------------------------------------------- Captured log call --------------------------------------------------------------------------------------------
WARNING  aroma.aroma:aroma.py:50 Output directory /home/vicente/nilearn_data/development_fmri/development_fmri/out exists and will be overwritten.
================================================================================= 1 failed, 3 passed in 152.12s (0:02:32) =================================================================================

Did you forget to eliminate this line or this file is supposed to be generated and now it isn't?

vinferrer commented 4 years ago

@tsalo Could you take a look into this?

eurunuela commented 4 years ago

Did you forget to eliminate this line or this file is supposed to be generated and now it isn't?

I've looked for the generation of that file and haven't found it. I guess we did not remove that assert. @tsalo may know better.

tsalo commented 4 years ago

The feature scores go to a tsv now, not a txt. If you change the extension, it should be fine. Here's the relevant lines:

https://github.com/Brainhack-Donostia/aroma/blob/59a21be69f37a3ce34fd77906998877c845287c0/aroma/utils.py#L303-L304

vinferrer commented 4 years ago

I am gonna modify the test_integration since previous changes make the join(resources_path, "classification_overview.txt") obsolete.

(Pdb) true_classification_overview.iloc[:, 1:]
    maximum RP correlation  Edge-fraction  High-frequency content  CSF-fraction
IC                                                                             
1                     0.65           0.65                    0.96          0.00
2                     0.86           0.66                    0.08          0.00
3                     0.89           0.62                    0.13          0.01
4                     0.62           0.80                    0.96          0.00
(Pdb) classification_overview.iloc[:, 1:]
    csf_fract  max_RP_corr       HFC classification
IC                                                 
0    0.004761     0.653423  0.962798       rejected
1    0.004964     0.867240  0.107143       rejected
2    0.005950     0.857130  0.094742       rejected
3    0.002666     0.608323  0.962798       rejected
vinferrer commented 4 years ago

Okay these changes should do it

vinferrer commented 4 years ago

@tsalo and @eurunuela. To sumarize. I had to change some files in the data folder since their structure changed in the lasts merges and changed the test_integration.py to make it work

tsalo commented 4 years ago

Can you post the original error that this fixes? It seems like the scope of the PR has increased and I want to make sure it's fixing the bug @eurunuela noticed in https://github.com/Brainhack-Donostia/aroma/pull/25#issuecomment-725899407.

vinferrer commented 4 years ago

Can you post the original error that this fixes? It seems like the scope of the PR has increased and I want to make sure it's fixing the bug @eurunuela noticed in #25 (comment).

test_integration.py:31: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../aroma.py:243: in aroma_workflow
    motion_ICs = utils.classification(features_df, out_dir)
../utils.py:316: in classification
    proj = HYPERPLANE[0] + np.dot(x.T, HYPERPLANE[1:])
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

args = (array([[0.65333356, 0.86082873, 0.88353643, 0.61168498, 0.43822201,
        0.405519  , 0.52083742, 0.21823561, 0.790..., 0.4905189 , 0.47750809,
        0.47114375, 0.36552243, 0.4786795 , 0.57517467]]), array([ 9.95127548, 24.83331602]))
kwargs = {}
relevant_args = (array([[0.65333356, 0.86082873, 0.88353643, 0.61168498, 0.43822201,
        0.405519  , 0.52083742, 0.21823561, 0.790...05189 , 0.47750809,
        0.47114375, 0.36552243, 0.4786795 , 0.57517467]]), array([ 9.95127548, 24.83331602]), None)

>   ???
E   ValueError: shapes (2,44) and (2,) not aligned: 44 (dim 1) != 2 (dim 0)

<__array_function__ internals>:5: ValueError
-------------------------------------------------------------------------------------------- Captured log call --------------------------------------------------------------------------------------------
WARNING  aroma.aroma:aroma.py:50 Output directory /home/vicente/nilearn_data/development_fmri/development_fmri/out exists and will be overwritten.

Yeah i think is the same error, that's solve by changing x.T to x

vinferrer commented 4 years ago

We should finish this today. I got a few PR waiting for this

vinferrer commented 4 years ago

The tsv change is done.

vinferrer commented 4 years ago

Yes but we need the main branch to work properly