populse / populse_mia

Multiparametric Image Analysis
Other
10 stars 9 forks source link

[Bug of automatic completion] Auto-completion of lists overrides parameters entered by user #269

Closed LStruber closed 2 years ago

LStruber commented 2 years ago

Note: This has only be tested with controller V2

Minimum step to reproduce:

The brick is launched but some of the parameters are reinitialized to their default values in the controller during automatic completion ! See image below.

Default Coregister brick controller: coregister_default

Modified coregister brick controller: coregister_changed

Coregister brick controller when running: coregister_after_completion

The brick is indeed launched with false (reinitialized) parameters ! This is very problematic :/

@servoz I needed to initialize some bricks by this mean in MRIQC pipeline, but as it is not working, I will hardcode these parameters in the brick, which then will become less adaptable (but working)

servoz commented 2 years ago

Oh damn... This is really a big bug !!!! We need to fixe it ASAP.

servoz commented 2 years ago

I reproduce!

The run was actually launched with the fwhm == [3, 6, 6] parameter as we can see in soma-worflow and in the history window Screenshot from 2022-04-23 19-20-51

When we change the value, the pipeline became blue, i had a look in the capsul pipeline_developer_view module, but i haven't found what this blue colour is (it doesn't seem to be a good sign!).

During these tests, I observed another issue that is related, I think, to the work on the history window. There is now an entry in the History tag with the history button for nipype script file. Screenshot from 2022-04-23 19-35-27 1/ this script should not be indexed in the project (it's an internal code file that doesn't interest the user, although this can be discussed, by the way I don't remember if we made sure not to index it like we do with the mia_processes bricks?). 2/ but more importantly I think that there shouldn't be the button to access the history for this file. I will open another ticket about this as soon as possible.

servoz commented 2 years ago

Hum ... I just tested with the V1 controller... I did not observe this problem with the old controller...

So, I think the issue is strongly related to V2 controller (I think we have to investigate mainly in capsul side).

And @LStruber , maybe it is not necessary to hard code in the brick, it will be enough to use the V1 controller while waiting to fix this ticket? (I also have bricks that only work with controller V1 because of the lack of support for complex types with controllerV2).

Can you also test your bricks with the V1 controller and say if you don't see this issue with this controller too?

LStruber commented 2 years ago

And @LStruber , maybe it is not necessary to hard code in the brick, it will be enough to use the V1 controller while waiting to fix this ticket? (I also have bricks that only work with controller V1 because of the lack of support for complex types with controllerV2).

I think I will implement both for the sake of simplicity (for users). One simple brick with hardcoded parameters, because the user may want to perform a "simple" registration that works in most cases (without defining parameters), and a complex registration brick with all inputs avalaible to the user.

I'll test the brick with V1 controller asap.

servoz commented 2 years ago

I think I will implement both for the sake of simplicity (for users). One simple brick with hardcoded parameters, because the user may want to perform a "simple" registration that works in most cases (without defining parameters), and a complex registration brick with all inputs avalaible to the user.

Why not, but as you know you can define the default values you want for the parameters in the constructor of the class corresponding to the brick (when self.add_trait("foo", ...)). I think it's like if you hard code the value (the user can use as it is the brick) and in addition there is the possibility to change the value if the user wants. This is the idea in which the bricks in mia_processes have been coded for the moment. But as you want!

LStruber commented 2 years ago

You're true but I would like to have 2 default values for this brick, one for T1w file and the other for bold files. That's why I think it could be good to get a easy "T1wRegistration", a easy "BoldRegistration" and a complex "Registration", but that is of course debatable

servoz commented 2 years ago

You're true but I would like to have 2 default values for this brick, one for T1w file and the other for bold files. That's why I think it could be good to get a easy "T1wRegistration", a easy "BoldRegistration" and a complex "Registration", but that is of course debatable

Ok, but ... :-))) there is also the possibility to set values when building the pipeline! Is this what you want? For example in the spatial_preprocessing pipeline we define for the normalize12_2 brick, the plug write_voxel_sizes = [2.0, 2.0, 2.0].

LStruber commented 2 years ago

I didn't know that ! That would indeed solve the problem I guess. I'll try this way

denisri commented 2 years ago

(I'm coming back from vacation so I haven't looked at this ticket before) Yes it seems to be a bug in the Controller GUI. It isn't related to completion: if you modify values in a list as you did, then open a node controller GUI on the same node, you get the initial values - or more exactly the 1st values of lists seems to be modified, not the other ones. I'll try to have a look.

denisri commented 2 years ago

It seems even more complex than that: if I instantiate the process (Coregister for instance, here) using the standard Capsul API, then open some ControllerWidgets on it, I don't see this desynchronization. But in Populse_mia, after a couple of values are changed, further modifications have no effect, as if callbacks stopped firing at tome point. When I click again on the same node, older values are reset, then I can edit a couple of new values (which are actually taken into account and reported to other controller views) for a new couple of edits, then it desynchronizes again, and so on.

denisri commented 2 years ago

I think I could fix the problem in soma-base. Could you try it on your side please ?

servoz commented 2 years ago

Sure! I do it!

servoz commented 2 years ago

Oh yes, the issue is now fixed. Thanks @denisri!