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

[bricks in progress] EstimateModel and EstimateContrast improvement #22

Closed
manuegrx opened this issue Apr 25, 2023 · 9 comments
Closed
Assignees

Comments

@manuegrx
Copy link
Contributor

EstimateModel and EstimateContrast bricks need to be improve in order to be universal and to work with all fMRI data

@manuegrx manuegrx self-assigned this Apr 25, 2023
@servoz servoz changed the title bricks/stat/spm improvement - EstimateModel and EstimateContrast [new bricks to do] EstimateModel and EstimateContrast improvement May 2, 2023
@servoz servoz changed the title [new bricks to do] EstimateModel and EstimateContrast improvement [bricks in progress] EstimateModel and EstimateContrast improvement May 2, 2023
@servoz
Copy link
Contributor

servoz commented May 17, 2023

When the bricks will be improved, don't forget to make the documentation.

@manuegrx
Copy link
Contributor Author

manuegrx commented May 22, 2023

In order to be able to run level1design, EstimateModel and EstimateContrast bricks for several subjects in a single project it seems necessary to add a sub-folder for the SPM.mat file and all others outputs (beta images, contrasts images .. ).

For Level1design brick, it seems sufficient to add a out_dir_name input.
The SPM.mat file will be automatically created in the directory : ../data/derived_data/out_dir_name.
It is done using spm_mat_dir input in Nipype process.
If this input is set to Undefined (default), we used the subjects names in the MIA database and out_dir_name = stat_subjects_names (for ex : ../data/derived_data/stat_sub-08)

For EstimateModel and EstimateContrast it is a little bit more complicated because there are no input in the Nipype process for the output folder.
In Nipype, the outputs are automatically created in the SPM.mat folder.
As we are using Capsul, the output are created in the self.output_directory even if the input SPM.mat file is in the folder self.output_directory/stat_subjects_names (because the temp folder is created in self.output_directory and then the output are move to self.output_directory).

To bypass this issue, I set self.output_directory to os.path.join(self.output_directory, self.out_dir_name) at the begging of the process.
It is working but I think it not a good idea to keep it that way because, for example, if the user use the same pipeline in the interface it will create a folder self.output_directory/self.out_dir_name/out_dir_name.

Others options could be:

  • move the outputs created in self.output_directory to the folder self.output_directory/self.out_dir_name/ after the run, I do not know if it is okay to do it that way.

  • try to add in Capsul or in populse an option to copy the output not in 'self.output_directory' but in self.output_directory/self.out_dir_name/. For this option I think I will need your advice @servoz

@servoz
Copy link
Contributor

servoz commented May 22, 2023

A subdirectory of self.output_directory to differentiate results from different subjects/patients within a single project is becoming an increasingly important need.

This need has already been explored for the CVR pipeline. For example, in bricks.preprocess.others.Resample_2.
In this case, the PatientName tag is mandatory in the database in order to create the directory that will allow to differentiate the results between several subjects (self.output_directory + "/roi_" + patient_name is used but this name can be modified). This works well for pure mia_process bricks.

However, the problem becomes more complicated when bricks come from nipype/SPM bricks. There are indeed different machineries to take into account. SPM wants to put the results where SPM.mat is. Capsul wants to put the results in self.output_directory.

I don't have the final solution, we'll have to study this issue quickly and have a global vision because the results of the SPM statistical bricks are used in the rest of the CVR pipeline (so the output changes for the SPM statistical bricks will have an impact on several other bricks).

I don't remember exactly how self.output_directory is created and managed in capsul - populse_mia. It seems to me that we could keep the self.output_directory coming from capsul and modify it at each initialisation and run of a process in mia_processes.
I don't remember exactly the codes for these parts in caspul and propulse_mia. We'll have to do some introspection. Firstly populse_mia, mainly the user_interface_pipeline_manager.process_mia module.

Beside that I'll be surprised that @denisri didn't have this need ? If yes how did you managed it @denisri ?

@manuegrx, we can discuss this at our meeting tomorrow.

@servoz
Copy link
Contributor

servoz commented May 24, 2023

Following our meeting yesterday it seems that the populse_mia machinery allows to use a subdirectory in output_directory.

In some cases it would be necessary to store the results in a subdirectory in order to discern the results for several subjects/patients (e.g. if the process always uses the same output name - case of SPM.mat in SPM statistics processes - and when we want to iterate the pipeleine on several patients).

I think the best would be to manage the subdirectory outside mia_processes (the process) at a higher level, I think populse_mia would be suitable. Before implementing this in populse_mia, we should think about the implications and side effects.

A good solution might be to start by seeing what happens by doing it in mia_processes, so we can start testing the idea and see what happens.

I also need to create output subdirectories in the CVR pipeline.

We could start with a schema like output_directory/sub_name/foo1/foo2, where foo1 and foo2 would be managed at the process level and sub_name for the moment at the process (mia_processes) level then eventually by populse_mia.

The question is what do we take as sub_name ?

The first naive idea that comes to me is the patient/subject ref.
The first side effect is that the PatientName tag must be filled in the database.
We can decorate the PatientName as data_PatientName.

I'm currently rewriting a lot of parts of the CVR pipeline, precisely on parts that need output_directory/sub_name.

So we need to decide ASAP what to use (so we don't have to change it later if possible) in order to use the same standard.

PatientName (or data_PatientName, or whatever!) is good for you @manuegrx ?

@servoz
Copy link
Contributor

servoz commented May 24, 2023

I started using on my side:
PatientName_data/foo1/foo2

@manuegrx
Copy link
Contributor Author

@servoz good for me !
I will use this for spm stat

@manuegrx
Copy link
Contributor Author

manuegrx commented Jun 8, 2023

This issue is still a WIP, but here is a quick update :

  • set self.output_directory to os.path.join(self.output_directory, 'data_PatientName') is not working correctly with iteration, an other ticket will be created soon about this issue . Regarding EstimatedContrast and EstimateModel bricks, I will first improve the bricks without used a sub directory (so it will work only for one subject for now)

  • for level1design: When model derivatives are used for HRF, more beta are created. I added a "multiplier" to take this case into account.

  • for EstimateModel : When factor_info parameter is used in Level1design brick, T and F contrasts (ess*, con*, spmF* and spmT* images) are created automatically by SPM. As those outputs are not defined in Nipype, Capsule/populse do not copy those outputs from the temporary folder to the final output directory.
    I added this outputs on my local Nipype and It is working, I will make a Nipype PR
    Once this PR done, the brick EstimateModel should work for all case for Classical method
    (EDIT : Missing contrast outputs for SPM EstimateModel nipy/nipype#3576 and add contrast outputs for EstimatedModel  nipy/nipype#3577

  • for EstimateContrast : still in progress, for now it is not working correctly for F contrast

@manuegrx
Copy link
Contributor Author

Update completed in a696172

For EstimateContrast, in Nipype it is necessary to define first T-contrast before to define a F-contrast.
So the brick have been updated to work for F-Contrast (now T and F contrasts are not define in the same input)

There is still few issues:

@manuegrx
Copy link
Contributor Author

manuegrx commented Jul 5, 2023

I added a ticket in mia for the Nipype issue #47 --> This ticket can be close, all the issue are in others tickets

@manuegrx manuegrx closed this as completed Jul 5, 2023
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

No branches or pull requests

2 participants