Convert pred data to uint8 prior to imwrite png

[mathieuboudreau]

Checklist

GitHub

• I've given this PR a concise, self-descriptive, and meaningful title
• I've linked relevant issues in the PR body
• I've applied the relevant labels to this PR
• I've assigned a reviewer

PR contents

• I've consulted ivadomed's internal developer documentation to ensure my contribution is in line with any relevant design decisions
• I've added relevant tests for my contribution
• I've updated the relevant documentation for my changes, including argparse descriptions, docstrings, and ReadTheDocs tutorial pages

Description

Writing the histology segmentation predictions throws a warning:

Lossy conversion from float64 to uint8. Range [0, 1]. Convert image to uint8 prior to saving to suppress this warning.

The root cause is that nibabel's fdata() call forces the returned data to always be float64, while we're saving to uint8.

This quick fix resolves that and gets rid of the warning.

Linked issues

External:

Axondeepseg

• Improve and force imread/imwrite conversion to 8bit int axondeepseg/axondeepseg#669
• Resolve warning during segmentation about lossy conversion (int64 to uint8) axondeepseg/axondeepseg#175
• Suppress warnings axondeepseg/axondeepseg#666

[mariehbourget]

Thanks @mathieuboudreau!
I tested the PR without binarizing the predictions (to have some gray values) and compared them with master and the predictions are identical.

However, I noticed that pred_to_png is also used with the comand --test to output "painted" versions of the predictions (with a different color for TP, FP, FN "objects") here.
The "colors" are set at values 1, 2 and 3 here.

With the imageio conversion, the values 0-1-2-3 were scaled between 0-255 at 0, 85, 170 and 255.
But with the changes in this PR, all the values end up at 255.

Honestly, this feature is useful for spinal cord of lesions segmentation on MRI but is of very little interest for axon and myelin segmentation, so I wonder if we should just remove it for microscopy.

Funny enough, I started working a while ago on another PR #1081 to skip the evaluation of TP, FP and FN objects and "painted" files for the axon and myelin task.

Let me revisit my old PR and think about this and I'll get back to you.

[mariehbourget]

I tested the PR without binarizing the predictions (to have some gray values) and compared them with master and the predictions are identical.

However, I noticed that pred_to_png is also used with the comand --test to output "painted" versions of the predictions (with a different color for TP, FP, FN "objects") here. The "colors" are set at values 1, 2 and 3 here.

With the imageio conversion, the values 0-1-2-3 were scaled between 0-255 at 0, 85, 170 and 255.
But with the changes in this PR, all the values end up at 255.

Honestly, this feature is useful for spinal cord of lesions segmentation on MRI but is of very little interest for axon and myelin segmentation, so I wonder if we should just remove it for microscopy.

@mathieuboudreau
Thinking out loud here.
I would rather not remove the "painted" option entirely in case we have tasks in the future with microscopy for which it is useful (task other that axon/myelin seg).

Maybe a good way to prevent issues with the conversion while remaining flexible to different scenarios could be to add a "range" argument to pred_to_png.

So everytime we call pred_to_png, we could specify the range (either [0-1] or [0-3]) and do the conversion to [0-255] accordingly. What do you think?

[mathieuboudreau]

What do you think?

I'm ok with that! For simplicity, may I suggest that the function argument be something like num_classes instead of range? This way you can pass on a single value (also, one thing as a user that always make me think twice is that some python functions exclude the last value in a range, i.e.:

>>> import numpy as np
>>> a=np.arange(0,3)
>>> print(a)
[0 1 2]

so users risk passing on on the wrong "range" sometimes if they are also confused on if the last value is excluded or not).

[mariehbourget]

For simplicity, may I suggest that the function argument be something like num_classes instead of range?

I agree that range can be misleading.
But num_classes can be misleading as well, for example we have a 2-class training (axon and myelin) but each predictions has only a range of 0-1.
Maybe max_value?

[coveralls]

Pull Request Test Coverage Report for Build 3333750457

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 69.04%

---

Totals
Change from base Build 3143352651:	0.0%
Covered Lines:	4237
Relevant Lines:	6137

---

💛 - Coveralls

[mathieuboudreau]

@mariehbourget I implemented your suggestion, but am unsure if the tests cover it well because it's only called in integration tests I think and not a function-specific unit test. Would you mind taking a look, or provide me with information on how to test this using your example of a 3 class dataset?

[mariehbourget]

I think we have a misunderstanding in one of our previous discussion.
The max_value here is 1, no matter the number of classes.

pred_to_png is used at 3 different places in the ivadomed code:

• Here + in main.py where the values of the pred list are predictions in float between [0-1], for each class, so the conversion is not related to the number of classes.
• In evaluation.py where the pred_list is a "painted" list (painted according to TP FP FN objects) in float with range [0-3].

So with a default max_value of 1, we don't have to specify the max_value here and we get the same behavior as before.
However, we need to specify the max_value=3 in evaluation.py to have the correct output of the "painted" predictions.

[mathieuboudreau]

Reminder: https://ivadomed.org/tutorials/two_class_microscopy_seg_2d_unet.html

[mariehbourget]

Hi Mathieu,
As discussed in meeting, I reviewed the PR and added 1 suggestion to deal properly with the "max_value" of "painted data".

For pred_to_png itself, the behavior with the new conversion is correct and as expected.
I will add a procedure for testing the PR shortly.

[mariehbourget]

Here is a procedure to test the PR and to make sure that results are the same as with the master branch.
@mathieuboudreau let me know if you have any questions or concerns, thanks!

1. Create a log folder.
2. Copy the model_seg_rat_axon-myelin_sem.pt file from the SEM model in the log folder.
3. Rename the file best_model.pt.
4. Copy the config file model_seg_rat_axon-myelin_sem.json from the SEM model in the log folder.
5. In the config file, adjust the following:
   • path_output : the path of your log folder
   • loader_parameters/path_data : path to the SEM dataset
   • Replace the path in split_dataset/fname_split with null
6. Run ivadomed with the --test command: ivadomed -c path/to/your/config/file --test
7. In pred_masks in the log folder you should have 2 PNG predictions files and 2 PNG "painted" files:
   • sub-rat6_sample-data15_SEM_class-0_pred.png
   • sub-rat6_sample-data15_SEM_class-0_pred_painted.png
   • sub-rat6_sample-data15_SEM_class-1_pred.png
   • sub-rat6_sample-data15_SEM_class-1_pred_painted.png
8. Predictions files should have 2 different values: 0 (background) and 1 (axon or myelin)
9. Painted files should have 4 values: 0 (background), 85 (True Positives), 170 (False Positives) and 255 (False Negatives)
10. Make sure that the results are the same on this branch vs. master branch.

As a reference, here are examples of TP, FP and FN on the painted files:

[mathieuboudreau]

@mariehbourget I tested your (very well written!) testing procedure on 1) the master branch 2) my branch prior to your changes, and 3) the branch after your changes, and can confirm that your suggested changes have fixed the issue and now reproduced master. It looks good to me, so if you're ok with it I suggest you approve this PR and merge.

After that, I have it in my notes and the ADS action tasks that we should ask IVADOMED makes a new release with this update as soon as it's convenient for you, so that it would be included in our installed the version of IVADOMED installed with the ADS dependencies.

[mariehbourget]

After that, I have it in my notes and the ADS action tasks that we should ask IVADOMED makes a new release with this update as soon as it's convenient for you, so that it would be included in our installed the version of IVADOMED installed with the ADS dependencies.

Yes, if I remember correctly, the goal of having a new ivadomed release was to also be able to implement the "no-patch" option from a previous PR (#1101) directly in ADS (axondeepseg/axondeepseg#676).

I will merge this PR and setup for a release next week if it's ok with the ivadomed dev team.
Thanks @mathieuboudreau!