Add check_bbox argument management

[EmmaRenauld]

Very small fix, remove bounding box valid checks.

[AntoineTheb]

Personnellement je mettrais ça partout dans scilpy, mais c'est en effet une bonne idée d'ajouter ça au moins au scoring.

[arnaudbore]

Build passed ! Good Job 🍻 !

[arnaudbore]

Ok can you tell me a little bit about why this is such an issue ? Why we don't add a new arg to force bbox_valid_check to false ?

[EmmaRenauld]

I'm not sure in other scripts, but here we are scoring tractograms that are potentially bad.

But if I try to think about general cases, I would say the most probable case is when vertical bundles/streamlines go too far in the brainstem under the pons, into the spinal cord, they can go out of the image quite easily. Other ideas, @AntoineTheb ?

[AntoineTheb]

Yeah that's usually the problem. The FiberCup's ground truth also goes out its volume.

[AntoineTheb]

Thinking about it, it could probably be a good idea to add an option to the argparser to disable bbox checking (and make it easily available to other scripts, like the --verbose or --force option for example). I'll add an issue to remind myself to do it @arnaudbore @EmmaRenauld .

[arnaudbore]

I agree, I don't like to automatically skip the bbox checking.

[EmmaRenauld]

I agree too.

[EmmaRenauld]

I made the change in all scripts using that check. Tested the whole pytest locally, it works.

During loading: when using load_tractogram_with_reference, you don't need to send the info, it will get it in bbox_check in the args, the same way as is done with the ref.

During saving: we use bbox_valid_check=args.bbox_check.

[AntoineTheb]

Cool ! Do you have a tractogram to test it ?

[arnaudbore]

Build passed ! Good Job 🍻 !

[arnaudbore]

Build passed ! Good Job 🍻 !

[EmmaRenauld]

scoring_data=$BrainData/databases/ismrm_2015/derivatives/scoring_data/output/version_2022
ref=$scoring_data/t1.nii.gz
config_file_segmentation=$scoring_data/config_file_segmentation.json
config_file_tractometry=$scoring_data/config_file_tractometry.json
submissions_dir=$BrainData/databases/ismrm_2015/derivatives/submissions/output/anonymized_submissions_converted_tck

out_dir_root=tmp/
mkdir $out_dir_root

tractogram=$submissions_dir/3_3.tck
team=3_3
out_dir=$out_dir_root/$team;

# ------ Segmentation -------
scil_score_tractogram.py -f -v $tractogram $config_file_segmentation $out_dir --no_empty \
          --gt_dir $scoring_data --reference $ref --json_prefix tmp_  # ----------------> Fails

scil_score_tractogram.py -f -v $tractogram $config_file_segmentation $out_dir --no_empty \
          --gt_dir $scoring_data --reference $ref --json_prefix tmp_ --no_bbox_check  # --------> works

# ------ Final scoring -------
scil_score_bundles.py -f -v $config_file_tractometry $out_dir \
            --gt_dir $scoring_data --reference $ref;  # ----------------> Fails

scil_score_bundles.py -f -v $config_file_tractometry $out_dir \
            --gt_dir $scoring_data --reference $ref --bbox_check False;  # ------------------> Works.

[arnaudbore]

Build passed ! Good Job 🍻 !

[arnaudbore]

Looking at all these modifications I think we have to really discuss about this implication because a lot of save and load were using bbox_valid_check=False and also now there is no distinctions between load and save and I don't really know if it's a good thing or not.

[arnaudbore]

missing add_bbox_check

[EmmaRenauld]

bbox_check=False was not there to start with. I just removed the unnecessary newline.

[AntoineTheb]

I'm not sure what you mean with "no distinction between load and save" @arnaudbore

[arnaudbore]

Build passed ! Good Job 🍻 !

[EmmaRenauld]

Maybe I can remove this big commit for now, just so that scil_score_bundles can be usable in the next release, and we keep this bigger change for later?

[arnaudbore]

@EmmaRenauld yes go !

[EmmaRenauld]

there is no distinctions between load and save and I don't really know if it's a good thing or not.

Same as Antoine. I'm not sure either what you mean. I did not change the way it already was. Even better, I removed the default "False" in many places to make it optional.

But to try and answer... In the case where you need to use it because you have a faulty tractogram, there is a very good change that if a streamline is misplaced in the input, it will also be in the output, no matter what the script does. So I would guess that it's ok in general.

[arnaudbore]

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-641/6/

[arnaudbore]

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-641/7/

[arnaudbore]

Build passed ! Good Job 🍻 !

[EmmaRenauld]

Is this ready, now that the release is done? @arnaudbore
I just rebased on master to verify.

[arnaudbore]

Build passed ! Good Job 🍻 !

[arnaudbore]

Build passed ! Good Job 🍻 !

[frheault]

@EmmaRenauld Can you update this code with master and then check if everything runs normally.
The master is now up to 3.10 (but should run in 3.8/3.9), so you may have to create a new venv.

[arnaudbore]

Build passed ! Good Job 🍻 !

[frheault]

Out of curiosity, how did you pick which script to change immediately in this PR? Are they all script that runs even with invalid?

[EmmaRenauld]

Hmmm.. it was 6 months ago so I don't exactly remember, sorry. I probably searched the word bbox_valid_check in all files.

[arnaudbore]

Build passed ! Good Job 🍻 !