populse / mia_processes

The default processes repository for mia
Other
1 stars 2 forks source link

[bricks in progress] Add contrast outputs for EstimatedModel in Nipype #47

Closed manuegrx closed 1 year ago

manuegrx commented 1 year ago

For EstimateModel brick (spm), 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.

An issue and a PR have been done in Nipype but not merged yet (it works with those modifications done in local).

If the modifications in Nipype are not merged, maybe it will be useful to better understand why Capsul/Populse do not copy these images.

servoz commented 1 year ago

Data created in the temporary directory, in the case of spm, should be transferred if the trait exists and has metadata output == True. We should therefore be able to manage things on our side.

Indeed, if the trait already exists in nipype it may make things easier, but I think that if everything is well defined in list_outputs() and run_process_mia() of the process it should work without nipype. I'm interested to see what happens (to fix the machinery if necessary).

What's more, as nipype PR and tickets management are fairly unpredictable, it would be nice to be able to manage this on our side too!!!!!! @manuegrx , can you create the brick you want ? (with the new outputs and value management in list_outputs() and the addition of traits in the self.process object in run_process_mia()).

I'll do a bit of introspection afterwards

manuegrx commented 1 year ago

I e-mailed you the data (swrafunc_all.nii and a rp file, to add in a project using the Add a document button) and here is the pipeline (with 2 bricks, leveldesign and EstimateModel): https://github.com/populse/irmage-tools-resources/blob/main/docs/examples/fmri_face_spm/pipeline_face_spm_categorical_stats.py

All the useful parameters should be already written in the pipeline (cond_onsets, factor_info..) and you should only filled "sess_scans" with swrafunc_all.nii and "sess_multi_reg" with the realignment file. image

At the end you should get 19 beta images, a spm.mat, 4 ess and 4 spmT images, 12 con and 12 spmF images, a ResMS.nii image, a mask.nii image and a RPV.nii image.

Let me know if something does not work ! In my side, without the modification in Nipype the ess, spmT, spmF and con images are in the MIA database but they do not exist in the output directory

If necessary, there is a quick tutorial here for theses data.

manuegrx commented 1 year ago

The PR in Nipype has been approved and merged ! @servoz I do not know if you want to keep trying to change things in MIA

servoz commented 1 year ago

Cool !

Have you tested that using nipype from source (while waiting for the next version) works as you wish with mia_processes on master?

Let's wait a bit before closing this ticket, I want to take advantage of the version of nipype without the PR to test on our side if we can manage the outputs if they are not defined in nipype (this could be useful in some cases).

manuegrx commented 1 year ago

Yes it works correctly using nipype from source with mia_processes on master !

servoz commented 1 year ago

OK, so the initial issue is now fixed !

I wanted to take advantage of this problem to explore the possibility of defining the output parameter on our side ... in vain.

The idea is simple. We define the corresponding trait in the process constructor (as we usually do). We manage the expected value of this parameter in the process's list_outputs() method (as we usually do, except that we usually only define the outputs dictionary object) and finally we add this trait to the self.process in run_process_mia() method().

But there's a leak ... as previously stated, data created in the temporary directory, in the case of spm, should be transferred if the trait exists and has metadata output == True ... but in this case at this time the trait has disappeared ... it's actually a case that we've never used and I've fought for several hours to find where this leak is happening (exact synchronisation with nypipe output and in our case it's not a nipype output ?), but I still haven't found were it comes from ... I'm not sure I have the time to spend too much time on this now that the PR has been accepted at nipype ???

I asked @denisri (in private message) where the leak could have come from (to see if there was a solution or if the idea was simply impossible).

For the moment, I've got something else to work on.

I suggest waiting a while to get @denisri opinion on the subject before closing this ticket.

servoz commented 1 year ago

Ok, I understood what was wrong, there is no leak, everything works perfectly ...

It didn't work because at the time of run_process_mia() the trait doesn't exist in self.process (it doesn't exist in nipype) and if we do self.process.foo = self.foo we don't pass the trait but its value, so it won't be present in process.user_traits().

I hadn't thought of this subtlety! So I instantiated the trait in question in the process and now it works properly .... except that at list_output() time in the mia_processes class we have no way of knowing the name of the temporary file chosen later by capsul ...

In short, my idea wasn't a good one!

As it is, we can't set an output value for an spm process from mia_processes if nipype forgot to set it... I think it would be possible by changing the way the temporary file part works on the capsul side, but I don't think it's worth it ...

If one day nipype doesn't make the necessary changes, we can consider such a change. For now, since nipype accepteed your PR @manuegrx , we've got more important things to do!