Added compatibility with uint8 to SSIM metric#3045
Conversation
There was a problem hiding this comment.
Thanks for working on this PR @MarcBresson !
I left a comment on how I would tackle this dtype problem.
I also label this PR with "help wanted" label to create a thread on our discord, where we could discuss about this PR in a more fluent way =>
EDIT: "Discuss "help-wanted" PR on Discord / discord (pull_request)" didn't work unfortunately, we'll see that later. However, if you would like to discuss this topic on discord, here is the link to the server: https://pytorch-ignite.ai/chat
ignite/metrics/ssim.py
Outdated
| if self.data_range != 255: | ||
| warnings.warn( | ||
| "dtypes of the input tensors are torch.uint8 but data range is not set to 255.", RuntimeWarning | ||
| ) |
There was a problem hiding this comment.
I'm not a big fan of showing a warning in this case. I would follow the same strategy as skimage where they just transform to floating point. So, I would do the following:
if not y.is_floating_point():
y = y.float()
if not y_pred.is_floating_point():
y_pred = y_pred.float()
There was a problem hiding this comment.
You are right, as data_range is a required argument in ignite (while it has a default value in skiimg), the warning is not required.
There was a problem hiding this comment.
Maybe we should put a default value to the data_range argument too? Most of the time we are dealing with [0; 1] images.
There was a problem hiding this comment.
I do not have a strong opinion on this. We have to figure out if there wont be any negative impact for the users in this case.
There was a problem hiding this comment.
I think the skiimg approach is quite good: default to data_range = 1.0, and warn if the detected type is of integer and that data range is still set to 1
|
@MarcBresson can you please resolve the conflict and let's apply what we discussed up to here: #3045 (comment) |
vfdev-5
left a comment
vfdev-5
left a comment
There was a problem hiding this comment.
Thanks @MarcBresson , LGTM!
vfdev-5
changed the title
|
Sorry for the delay btw! Got caught up in a job seeking activity ahah |
|
No worries, hope you can get something exciting as a new job role! |
2 participants
Add compatibility and tests for when y_pred and y are in uint8 dtype
Check list: