Fix edge case bug where grayscale image has an alpha channel, minor rtd fix

[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

Linked issues

#1312

[coveralls]

Pull Request Test Coverage Report for Build 7714334803

• 0 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.009%) to 74.278%

---

Totals
Change from base Build 7185147916:	0.009%
Covered Lines:	4678
Relevant Lines:	6298

---

💛 - Coveralls

[joshuacwnewton]

If I understand correctly, there are the following cases:

1. Grayscale (1-channel) -> [H, W] or [H, W, D]
2. Grayscale + alpha (2-channel) -> [H, W, 2] or [H, W, D, 2]
3. RGB (3-channel) -> [H, W, 3] or [H, W, D, 3]
4. RGB + alpha (4-channel) -> [H, W, 4] or [H, W, D, 4]

(And any of these cases could have an addition batch axis with N images.)

The code assumed that we would only have to deal with cases 1, 3, 4:

ivadomed/ivadomed/loader/segmentation_pair.py

Line 348 in 4c95d01

# 2. the colorspace is one of: binary, gray, RGB, RGBA (not aliasing ones like YUV or CMYK)

But we neglected to consider case 2 (grayscale + alpha)? (Hence the value error mentioning (744,1154,2), i.e. the last dim denotes a 2-channel image.)

[mathieuboudreau]

This PR is mostly ready for review, except for a test (and test image?) that we should add to cover line 377 which was the original bug source/fix.

[joshuacwnewton]

Just a few nitpicks for now :)

[kanishk16]

Idk why but it currently feels like a script to me rather than a module. Hence the suggested changes... More or less, the assert statements would not be required once we follow a flat hierarchy something along the lines:

colorspace_idx = 2

if _img.ndim <= colorspace_idx:
    pass
elif _img.shape[colorspace_idx] == 2:
    # gray + alpha
     ...
elif _img.shape[colorspace_idx] == 3:
    # RGB
     ...
  
  

[mathieuboudreau]

Idk why but it currently feels like a script to me rather than a module.

@kanishk16 not quite sure what you mean by this, is it because multiple steps aren't combined anymore? Personally, I prefer clarity at the cost of a few more lines, but I would really like this PR merged ASAP to help out our user in #1310 start training their data.

Are the changes you suggested mostly style preferences, or do you think they'll impact the performance? If it's the first part, can we push this through so the code becomes functional again now, and then open a new issue to discuss these stylistic details for this block of code?

If this isn't compromise you're willing to agree to, then I'd suggest we revert all the changes and use my single line bug fix for expediency: 340295d

[mathieuboudreau]

There's actually a second user currently wanting to do training using these datasets (the other one posted directly to ADS: axondeepseg/axondeepseg#783), so this PR is fairly high priority to get merged ASAP.

[mathieuboudreau]

@kanishk16 @joshuacwnewton I just reverted to my initial solution 340295d, because as I was doing the refactoring requested after @kanishk16 's point-by-point suggestions I noticecd that it simply was just converging back to this old code. The only additional change I maddewas the omission of the pointless "is_batch*.

Let me know if this is good for you

[kanishk16]

@mathieuboudreau I didn't know it was a high-priority PR... I'm going to pre-approve this PR irrespective of the solution you choose which can be discussed later...

[joshuacwnewton]

I'm good with reverting my suggestion! This has the benefit of making the diff of this PR smaller, making the changes easier to understand. :)

Pre-approving as well, apart from some nitpicks.

[joshuacwnewton]

More or less, the assert statements would not be required

Side note: I agree that the assertions definitely weren't required.

Thoughts on assertions

---

I like adding assertions as a way of clarifying the internal state of the code to anyone reading it. (Basically, testing the assumptions we're making about internal state. The assertions should be relevant mainly to developers, but generally should never be seen by users.) In other words, the assertions should never fire if our assumptions are correct? But, if our assumptions are incorrect (as they were with grayscale+alpha images), then the code will fail in an unambiguous way to make debugging easier.

As a specific example, over in SCT, we've started using assertions whenever there is a bare else: case:

# Anti-pattern: We assume that argument will be either option_1/2, but what if it isn't?
# Code will continue then probably crash at some unintuitive line of code
if argument == "option_1":
   pass
elif argument == "option_2":
   pass

# Slightly better: All cases are handled thanks to 'else:'
# However, the internal state is now ambiguous -- what is the value of 'argument' inside the 'else:'?
if argument == "option_1":
   pass
else:
   pass

# Preferred by SCT: All cases handled *and* assumptions + internal state are both clear
# If assumptions are correct, assertion statement will do nothing
# If assumptions are false, then the code will fail in a clear/unambiguous way
if argument == "option_1":
   pass
else:
   assert argument == "option_2", f"Unexpected value for argument: {argument}"
   pass

---

I think it might just be a personal style thing -- when I write code, much like my writing in general, I tend to be pretty verbose? 😅

[kanishk16]

Out of all the comments, maintaining a flat hierarchy is significant as it usually improves performance and in the case of training large data, this could become relevant...

[mathieuboudreau]

Thanks guys for all the feedback and work put into this! We ended up walking in a circle a bit but a little bit of exercise is not always bad =) haha

[kanishk16]

I guess in all this I forgot to ask one important ques, do we want to add the test img to this PR?

[mathieuboudreau]

I guess in all this I forgot to ask one important ques, do we want to add the test img to this PR?

Yup! Just did in that repo, will restart the tests now to see if there is increased coverage

[kanishk16]

We'd have to cut a release for it and add the new release tag here:

ivadomed/ivadomed/scripts/download_data.py

Line 26 in 4c95d01

"url": ["https://github.com/ivadomed/data-testing/archive/r20220328.zip"],

Let me know if you want me to cut a release!

[mathieuboudreau]

We'd have to cut a release for it and add the new release tag here:

ivadomed/ivadomed/scripts/download_data.py

Line 26 in 4c95d01

"url": ["https://github.com/ivadomed/data-testing/archive/r20220328.zip"],

Let me know if you want me to cut a release!

Ah gotcha - right, that's the same workflow we have in ADS. I just did it, let see how the tests go!

[mathieuboudreau]

Line 357 got hit per the coverage report

Merging - thanks for the help @joshuacwnewton and @kanishk16 !

[joshuacwnewton]

Thank you for taking care of the PR, @mathieuboudreau & @kanishk16. ♥️

((And, my apologies for taking up time on this PR by proposing a rewrite! Thank you for your patience.))

[kanishk16]

Thank you @mathieuboudreau, for all the patience while I stalled the PR & @joshuacwnewton for helping along.

---

I'd learn smthg.

I like adding assertions as a way of clarifying the internal state of the code to anyone reading it. (Basically, testing the assumptions we're making about internal state. The assertions should be relevant mainly to developers, but generally should never be seen by users.) In other words, the assertions should never fire if our assumptions are correct? But, if our assumptions are incorrect (as they were with grayscale+alpha images), then the code will fail in an unambiguous way to make debugging easier.

I completely agree with the above idea regarding assertions, but ... let's tackle it as per the case:

# Anti-pattern: We assume that argument will be either option_1/2, but what if it isn't?
# Code will continue then probably crash at some unintuitive line of code
if argument == "option_1":
   pass
elif argument == "option_2":
   pass

On point! ✔️

# Slightly better: All cases are handled thanks to 'else:'
# However, the internal state is now ambiguous -- what is the value of 'argument' inside the 'else:'?
if argument == "option_1":
   pass
else:
   pass

✔️

# Preferred by SCT: All cases handled *and* assumptions + internal state are both clear
# If assumptions are correct, assertion statement will do nothing
# If assumptions are false, then the code will fail in a clear/unambiguous way
if argument == "option_1":
   pass
else:
   assert argument == "option_2", f"Unexpected value for argument: {argument}"
   pass

This is where I feel assertion isn't the right way to go... WDYT about this:

if argument == "option_1":
   pass
elif argument == "option_2"
   pass
else:
  raise ValueError(f"Unexpected value for argument: {argument}")

...
# some in-place operations on argument
...
# now use assert to clarify the internal state before using it for some other operations

---

I think it might just be a personal style thing -- when I write code, much like my writing in general, I tend to be pretty verbose? 😅

Nah, I didn't mean in that sense... think about it this way... pretty much in ivadomed there were too few comments, and suddenly, there's code along with comments...

I like your writing style, and I'd like to believe being verbose is your strength that brings your thoughts alive ❤️