populse / populse_mia

Multiparametric Image Analysis
Other
10 stars 9 forks source link

[iteration] Issue when using ReduceNode brick #309

Closed manuegrx closed 1 year ago

manuegrx commented 1 year ago

Describe the bug As explain in the populse_mia procedure, if the parameter to be iterated on is a list, we need to use a node which transform a single file (input for this iteration) into a list of just one element in order to disambiguate the “list of list” situation in the iteration. It is possible to use “capsul > pipeline > custom_nodes > reduce_node > ReduceNode” or the “mia_processes > bricks > tools > Files_To_List” brick.

When the iteration is done via input filter brick/process it seems that the ReduceNode brick is used.

When I iterate either via a regular iterative pipeline using ReduceNode brick or via input filter brick/process I have an issue with the ReduceNode brick.

If I iterate via a regular iterative pipeline using Files_To_List brick it is working.

Generated error

Traceback:
  File "/home/gourieuxe/apps/sources_populse/populse_mia/populse_mia/user_interface/pipeline_manager/pipeline_manager_tab.py", line 1544, in initialize
    self.test_init = self.init_pipeline()
  File "/home/gourieuxe/apps/sources_populse/populse_mia/populse_mia/user_interface/pipeline_manager/pipeline_manager_tab.py", line 1649, in init_pipeline
    missing_mandat_param = self.get_missing_mandatory_parameters()
  File "/home/gourieuxe/apps/sources_populse/populse_mia/populse_mia/user_interface/pipeline_manager/pipeline_manager_tab.py", line 1484, in get_missing_mandatory_parameters
    if node.context_name.split(".")[0] == "Pipeline":
AttributeError: 'ReduceNode' object has no attribute 'context_name'

(error in get_missing_mandatory_parameters function)

To Reproduce Steps to reproduce the behavior via a regular iterative pipeline:

  1. Open a project with at least 2 subjects / 2 documents in order to do an iteration
  2. In pipeline manager tab, add the smooth brick (mia_processes/bricks/preprocess/spm), export in_files parameter using ReduceNode brick , export output parameters: image
  3. Check on the iterate pipeline button and click on "OK" (without checking in_files). It will create the pipeline to iterate image
  4. Select inputs node and add the in_file, run the pipeline . The error is in the standard output.
servoz commented 1 year ago

Thank's @manuegrx for this very clear ticket and with the To reproduce step that will help for the investigation.

It is not impossible that the problem comes from the mia side (change we made, because this capsul > pipeline > custom_nodes > reduce_node > ReduceNode worked before, if I remember correctly) and not from the brick side.

I'm on another case and will look into it ASAP.

denisri commented 1 year ago

context_name is an attribute set in pipeline nodes to provide them with their full name inside the pipeline (pipeline.subpipeline.node_name for instance). It is not totally mandatory in nodes (the attribute does not exist upon construction) but is normally set by Pipeline methods add_process_node(), add_switch() etc. But it had apparently been forbidden in add_custom_node(), and ReduceNode is a custom node, which explains the error. I have added it. However other code in Capsul does not consider it as mandatory, each access to it is done via something like getattr(node, 'context_name', node.name), not directly node.context_name. Well, can you pull the modif in capsul and try again ?

servoz commented 1 year ago

Oh that's it ! Thanks @denisri, we indeed used node.context_name instead of getattr(node, 'context_name', node.name) that is better. I will change every node.context_name to getattr(node, 'context_name', node.name) in mia ASAP.

manuegrx commented 1 year ago

Thanks @denisri ! However it seems that I have still the same issue after updating Capsul with your last commit !

Maybe we need to change node.context_name to getattr(node, 'context_name', node.name) as proposed by @servoz

servoz commented 1 year ago

Let's try again with a fresh master populse_mia, please. @denisri is right we must have a default value if in_context is not a process attribute. I'm in doing the necessary everywhere in mia, but for this ticket I think it's set, no?

denisri commented 1 year ago

Well right, in the GUI, nodes added in the PipelineDeveloperView were not added via the add_custom_node() method, and did not get a context_name. I've changed that. Now on MIA side I haven't checked how nodes are added in the pipeline, there may be missing things. Now using getattr() everywhere is a bit painful and not very elegant, I admit. Maybe we could set a default value or a property in Node, I'm not sure...

manuegrx commented 1 year ago

It is working with the update done in populse_mia thanks ! @servoz do you want to keep the ticket open until all the changes in mia are completed ?

servoz commented 1 year ago

I'll make the changes today and close this ticket if it's ok for you.

servoz commented 1 year ago

We use in Mia PipelineEditor.add_named_process() to add node/process in a pipeline. In there, we use the parent class method PipelineDeveloperView. add_named_process(). I haven't checked exactly what happens next, but I think it's OK. Can you confirm this, @denisri ? I'm finishing changing the context name retrieval everywhere. It's better this way.

denisri commented 1 year ago

Normally yes, context_name is set in Pipeline.add_process() which is called by PipelineDeveloperView. add_named_process().

servoz commented 1 year ago

For regular processes, the node name is foo_1, foo_2, etc. For custom nodes, it's foo1, foo2, etc.

In mia and mia_processes, we've adopted the foo_1 rule, I think because we're dealing with regular processes (which is why I mistakenly call them regular!).

I think it would be better to have a consistent name for all nodes? Would it be possible to change this for custom nodes (foo_n) in capsul ?

servoz commented 1 year ago

context_name fixed everywhere in populse_mia.

denisri commented 1 year ago

For regular processes, the node name is foo_1, foo_2, etc. For custom nodes, it's foo1, foo2, etc.

In mia and mia_processes, we've adopted the foo_1 rule, I think because we're dealing with regular processes (which is why I mistakenly call them regular!).

I think it would be better to have a consistent name for all nodes? Would it be possible to change this for custom nodes (foo_n) in capsul ?

OK, it's done.

servoz commented 1 year ago

Thanks!