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] Normalize12 spm from nipype is not working properly and result is wrong #29

Closed
servoz opened this issue May 16, 2023 · 6 comments
Assignees

Comments

@servoz
Copy link
Contributor

servoz commented May 16, 2023

it seems that the module in nipype is not correctly coded = > plan a PR at nipype ?
Review the coding of the brick in mia_processes.
Currently the result obtained is not the same as the one directly with spm ...

@servoz servoz changed the title Normalize12 spm from nipype is not working properly and result is wrong [bricks in progress] Normalize12 spm from nipype is not working properly and result is wrong May 17, 2023
@servoz servoz self-assigned this Aug 31, 2023
@servoz
Copy link
Contributor Author

servoz commented Sep 1, 2023

A PR has been opened on nipype side to correct the typos.

We should also take a look on our side, as we may also have an error in mia_processes. I'll looking into it.

@servoz
Copy link
Contributor Author

servoz commented Sep 1, 2023

Following the discussion in the PR, nipype launches correctly without going through populse, with both options mandatory=True, so there's no point in nipype changing this. We'll just have to live with it!

However, populse's test for missing mandatory parameters is based on this.

As a first approach, it could be that the nipype Normalize12 brick won't work in populse_mia, but that the mia_processes Normalize12 brick will work in populse_mia.
I'll take a look.

@manuegrx
Copy link
Contributor

manuegrx commented Sep 4, 2023

I have the same issue for an other brick (mrtrix DWIBiasCorrected ), two parameters are mandatory but exclusive !
If you find a way to make it work for Normalize brick, I'll do the same

@servoz
Copy link
Contributor Author

servoz commented Sep 4, 2023

If the PR is accepted, the output prefix will no longer be the hard-coded "w" but out_prefix (as expected)

servoz added a commit that referenced this issue Sep 4, 2023
servoz added a commit that referenced this issue Sep 4, 2023
@servoz
Copy link
Contributor Author

servoz commented Sep 4, 2023

I have the same issue for an other brick (mrtrix DWIBiasCorrected ), two parameters are mandatory but exclusive !

In mia_processes there is no problem because we can manage the value of the parameters before instantiation in the nipype process (see for example how it is managed in Normalise12, depending on the job_type we manage the mutually exclusive nature of the input parameters, if the user has not paid attention).

On the other hand, there is indeed a problem if we use a nipype process that is not overloaded in mia_process (by taking a brick from the nipype library directly into Mia's Pipeline Manager). In this case, the issue comes from the capsul's get_missing_mandatory_parameters.

In this method, there is no notion of mutually exclusive as defined by nipype, which has led to false positives (basically, if the parameter is not optional and has no value, the method declares it as missing).

We have two options:

  • Overload in Mia to modify the method (which is perhaps not the most desirable)

or

  • Modify in capsul.

In fact, there is a third option that we have always favoured up to now... It consists of saying that the nipype bricks are supplied as they are and that we are only committed to ensuring that the mia_processes bricks work. In this case, the problem is solved (we're a bit on the border line here because the problem actually comes from us, inside capsul ...).

Given the number of jobs to manage, I tend to prefer this third option... Maybe I'll open a ticket in capsul, just to remember this issue, and solve it as soon as we have a bit of time.

In a way, that's another problem. As far as this ticket is concerned, it seems to be working correctly now in mia_processes (the Normalise12 brick in mia_processes is now correctly working and results are fine) and I think we can now close this ticket

@servoz servoz closed this as completed Sep 4, 2023
@servoz
Copy link
Contributor Author

servoz commented Sep 6, 2023

A correction has been made to Mia for mutually exclusive parameters. Direct nipype processes (not encapsulated by mia_processes) are now correctly initialised with mutually exclusive parameters.

One last issue remains to be fixed, at least in Mia.
If the user makes a mistake and declares two mutually exclusive parameters, Mia crashes.
We need to catch the exception and not crash.

I'm going to open a ticket in Mia for this.

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