populse / populse_mia

Multiparametric Image Analysis
Other
10 stars 9 forks source link

[initialisation step] add the option of deleting a tag at the time of inheritance #310

Closed servoz closed 1 year ago

servoz commented 1 year ago

Following populse/mia_processes#39 ticket, it seems necessary to be able to delete a list of tags at tag inheritance time. We currently have the option of:

It seems necessary to also allow :

Indeed, in some cases, tags in foo1 make no sense for foo2.

When this option will be implemented before closing this ticket, don't forget to check that it allows to fix the second part of the ticket populse/mia_processes#39 ticket (in the example pipeline shown in this ticket, only SPM.mat should inherit from Contrast num). We should also change Level1Design for Regress num. etc.

servoz commented 1 year ago

We can now use the ProcessMIA.tags_inheritance() method instead of the inheritance_dict object when initialising bricks from mia_processes.

We can still use the self.inheritance_dict object, but in this case inheritance will only take place at the end of workflow generation. This is not at all desirable, and we'll have to abandon the use of the inheritance_dict object as soon as possible (in connection with the resolution of ticket #290 ).

In a nutshell:

tags_inheritance() takes the arguments:

This is a use case:

Pipeline with the same brick A twice : in -> A -> A -> out in file don't have "False tag 1" and "False tag 2" but have a value for "RepetitionTime". We want output of first brick A have the tag "False tag 1" == 101 and "False tag 2" == 1000 and the "RepetitionTime" value and out have "False tag 1" == 102 and "False tag 2" == 1000 and no value for "RepetitionTime" tag

all_tags_to_add = []

# Add a false tag in database
tag_to_add = dict()
tag_to_add["name"] = "False tag 1"
tag_to_add["field_type"] = "int"
tag_to_add["description"] = "A false tag"
tag_to_add["visibility"] = True
tag_to_add["origin"] = "user"
tag_to_add["unit"] = None
tag_to_add["default_value"] = None
false_tag = get_dbFieldValue( self.project, in_val, "False tag 1" )

if not false_tag:
    tag_to_add["value"] = 101

else:
    tag_to_add["value"] = false_tag + 1

all_tags_to_add.append(tag_to_add)

# Add a second false tag in database
tag_to_add = dict()
tag_to_add["name"] = "False tag 2"
tag_to_add["field_type"] = "int"
tag_to_add["description"] = "A false tag 2"
tag_to_add["visibility"] = True
tag_to_add["origin"] = "user"
tag_to_add["unit"] = None
tag_to_add["default_value"] = None
tag_to_add["value"] = 1000
all_tags_to_add.append(tag_to_add)

if false_tag:
    all_tags_to_del = []
    all_tags_to_del.append("RepetitionTime")

else:
    all_tags_to_del = None

self.tags_inheritance(in_val, out_val, own_tags=all_tags_to_add, tags2del=all_tags_to_del,)

The changes are in the populse_mia Fix#310 branch and the mia_processes FixPopulse_mia#310 branch.

I've started to make the necessary changes (remove self.inheritance_dict object and use of self.tags_inheritance() method), in the mia_processes FixPopulse_mia#310 branch only for mia_processes.bricks.preprocess. For afni, ants, dipy, freesurfer, fsl, and others, it's done. I haven't had time to change everything in spm yet. I'll continue after the 20.08.

@manuegrx , if you've got the time, you can start seeing if the fix suits you (the best would be for you to work on a new branch from the mia_processes FixPopulse_mia#310 branch). Feel free to keep modifying the bricks I haven't had time to modify yet, we can merge your branch into mia_processes FixPopulse_mia#310 when I get back from vacation after the 20.08).

I think we'll have to test this thoroughly before merging it into master (typo, error, ...)

servoz commented 1 year ago

The changes to use the tags_inheritance() method instead of the self.inheritance_dict object in all the mia_processes bricks are now complete.

I've tried to streamline things with the new way of working and replicate what was done before. The ability to delete tags now exists but I haven't used it. If necessary, as I think I've understood, the tags_inheritance() methods will have to be completed in this way for certain bricks (the stat bricks, see the end of the first post in this ticket).

I tested whether there were any issues following these changes with the CVR and MRIQC pipelines for anat and functional. I didn't observe any particular problems. There may be side effects that I haven't yet understood. We'll see with use.

I think this ticket can now be closed. I'm also going to merge the development branches with the master branches (mia_processes and populse_mia).

If we really want to do things properly, we're going to have to sort out the problem of inheritance for the other bricks that don't come from mia_processes. This will be the subject of the #290 ticket. I'll look at that ASAP.

If we really want to do well, we'll need to go through all the mia_processes bricks again carefully to make sure that the inheritance really makes sense (e.g. currently I think that for the Smooth brick, inheritance is done without changing the resolution after smoothing ... all the tags need to be consistent with what happened to the data in the past). A ticket is already open for this, populse/mia_processes#7. We should try to close it as soon as possible.

manuegrx commented 1 year ago

Perfect @servoz, I will have a look and use this new method to delete some tag in Level1Design brick !