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] Add contrast outputs for EstimatedModel in Nipype #47

Closed
manuegrx opened this issue Jul 5, 2023 · 7 comments
Closed

Comments

@manuegrx
Copy link
Contributor

manuegrx commented Jul 5, 2023

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
Copy link
Contributor

servoz commented Jul 5, 2023

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
Copy link
Contributor Author

manuegrx commented Jul 5, 2023

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
Copy link
Contributor Author

manuegrx commented Jul 6, 2023

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
Copy link
Contributor

servoz commented Jul 6, 2023

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
Copy link
Contributor Author

manuegrx commented Jul 6, 2023

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

@servoz
Copy link
Contributor

servoz commented Jul 6, 2023

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
Copy link
Contributor

servoz commented Jul 7, 2023

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!

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