Provide optional validation

[francois-rozet]

Is your feature request related to a problem? Please describe.
Currently, there is no way to skip the input validation (_validate_input) which is useful during prototyping and debugging but slows down the implementations. It could be interesting to add the option to skip the validation.

Describe the solution you'd like
With the flag -O, the Python interpreter doesn't consider assert statements. In fact, -O sets the global variable __debug__ to False. The following could be added to the start of _validate_input to shortcut all the validation when -O is used:

if not __debug__:
    return

Describe alternatives you've considered
Alternatively, a global variable _VALIDATION, a function _is_validation_enabled() and a function (or a context) _set_validation_enabled() could be provided.

def _is_validation_enabled() -> bool:
    return _VALIDATION

def _set_validation_enabled(mode: bool) -> None:
    _VALIDATION = mode

[zakajd]

Hi!
Yeah, validation part is not perfect now and we discussed with @snk4tr and @denproc recently that is should be simplified.
I like your solution with __debug__ variable.

Propose also do a bunch of improvements to speed up metrics:

1. Drop support for 2D and 3D tensors, and leave only 4D (and 5D for SSIM). This will make _adjust_dimensions function redundant. Users of the library are educated enough to read the docstring and cast inputs by themselfs if need. Especially after documentation is released (see Automatically build documentation for metrics  #179).
2. Drop support for scale_weights in form of tuple or list and make it mandoritary torch.Tensor. Drop huge part from _validate_input which is actually never used (LOL 😅 ) because we always call it with scale_weights=None:

if scale_weights is not None:
    assert isinstance(scale_weights, (list, tuple, torch.Tensor)), \
        f'Scale weights must be of type list, tuple or torch.Tensor, got {type(scale_weights)}.'
    if isinstance(scale_weights, (list, tuple)):
        scale_weights = torch.tensor(scale_weights)
    assert (scale_weights.dim() == 1), \
        f'Scale weights must be one dimensional, got {scale_weights.dim()}.'

3. Drop check for kernel size from _validate_input. It was initially introduced for SSIM, but now used only by SSIM, MS-SSIM and BRISQUE. We can specify in the docstring, that this value should be odd.

if kernel_size is not None:
    assert kernel_size % 2 == 1, f'Kernel size must be odd, got {kernel_size}.'

4. Drop some other pretty obvious asserts, because this function is only for internal use:

assert isinstance(input_tensors, tuple)
assert 0 < len(input_tensors) < 3, f'Expected one or two input tensors, got {len(input_tensors)}'
assert isinstance(tensor, torch.Tensor), f'Expected input to be torch.Tensor, got {type(tensor)}.' # We have type hints for that

What do you think?

[francois-rozet]

Hi @zakajd, those are great ideas! In fact, your type assertion would become very similar to what I just pushed to piqa. Having similar inputs, could help quite a lot if we were to merge the two projects 👍

For your information, here is the assertion function:

https://github.com/francois-rozet/piqa/blob/master/piqa/utils/__init__.py#L37-L87

And here is the way I used it in SSIM:

https://github.com/francois-rozet/piqa/blob/master/piqa/ssim.py#L288-L302

The reason why I used _is_assert_enabled() (which is a wrapper around __debug__) outside _assert_type() is to allow additional assertions, if needed by one of the metric.

Also, I do not perform value assertion (values within range) because it is O(N) and wrong values won't make the code crash. The way I handled the number of dimension is by giving a range of possible values instead a single one. This allows SSIM inputs to have as 4, 5 or even 42 dimensions. The same for PSNR, which isn't limited to 4D.

Moreover, I saw that you used Union[torch.Tensor, Tuple[torch.Tensor, torch.Tensor]] at a few places in the code base. I try to avoid that because Union prevents the components to be JITted. A valid replacement is List[torch.Tensor].

Finally, we might as well unify the way the outputs are reduced (the dictionary you use currently is duplicated everywhere). Also, is it necessary to add the reduction option to the functions ? I only added it to the classes because those are more likely to be used as criterions.

If you want, I can do a PR, where I modify _validate_input, _adjust_dimension and create _reduce_output 😉

[zakajd]

Let's wait for others to respond.

Considering _is_assert_enabled() function, let's keep validation part simple and hide __debug__ switch inside _validate_input. We can still have any number of additional asserts inside the metric, because use of -O flag removes those statements and any code conditional on the value of __debug__ doc

I like the idea with _reduce_output in classes. Copy-pasting same dict every time isn't very good idea. It's actually was even bigger mess before #92 somewhat standardised reduction process.

Value assertion is important, because passing tensors with [0, 255] scale to metric which expects it to be in [0, 1] can lead to huge drop in quality of "perceptual error" estimation . See #208 as a recent example.

Have you measured speedup between your's implementations with and without validation? In my experience, data loading and model inference takes much longer compared to loss computation, so squeezing milliseconds out of it is not always reasonable.

[francois-rozet]

Thx for the feedback, @zakajd.

Considering _is_assert_enabled() function, let's keep validation part simple and hide __debug__ switch inside _validate_input. We can still have any number of additional asserts inside the metric, because use of -O flag removes those statements and any code conditional on the value of __debug__ doc

Ok, I thought that assert was a function, but it is a syntactic sugar. Its "arguments" are not even read by the interpreter if -O is set. So I agree, no need for if _is_assert_enabled() 👍

Value assertion is important, because passing tensors with [0, 255] scale to metric which expects it to be in [0, 1] can lead to huge drop in quality of "perceptual error" estimation . See #208 as a recent example.
Have you measured speedup between your's implementations with and without validation? In my experience, data loading and model inference takes much longer compared to loss computation, so squeezing milliseconds out of it is not always reasonable.

It does make a difference. Computing the min and max takes about 0.4 seconds for 400 calls with a batch size (1, 3, 512, 512). This overhead is the same for all metrics. For slow metrics like LPIPS, it is not substantial, but for PSNR and SSIM, it makes the runtime, respectively, 10x and 2x longer. Still, I agree it is not a big deal. 1ms per iteration is not significant with respect to the image loading etc.

To summarize what is proposed:

1. Remove _adjust_dimension
2. Strengthen the input requirements
3. Simplify _validate_input and add if not __debug__: return at its start
4. Add function _reduce_output (or similar)

For the last one, is it necessary to add it to functions (psnr(), ssim(), ...) ? In piqa, I only implemented reduction for the classes (PSNR, SSIM, ...). It is just a choice of convention and can change it easily.

[zakajd]

Let's follow PyTorch design for losses, where reduction parameter is propagated to methods from functional.

This can be useful when computing mean metric value for dataset of images, not 1 - value as returned by many loss classes.

[francois-rozet]

Hello @zakajd, while waiting for the others, I've completely refactored piqa to follow what we said here, i.e.

• Type assertion (including shape, device and value range) + easy disable with -O
• PyTorch-like API: Object-Oriented components based on functionals
• Reduction propagated to functionals and _reduce function

However, it should be noted that

• Type assertion is only provided for the OO components as it is a high-level feature
• Functionals don't always do all the computation (color space conversion and down-sampling don't belong in there)
• Functionals are less user friendly, they require more arguments than OO counterparts (like kernel and weights)

What do you think ?

[snk4tr]

Hi @francois-rozet.

First, I would like to comment on the discussion about -O flag in Python and the removal/change of the _validate_input and adjust_dimensions functions.

I think that -O is indeed very useful and provides a significant performance boost. That is why we implemented virtually all validations using assert but not the if - raise pattern. Our users can easily speed up their execution in exchange for safety. However, further reduction of verification of user input looks not necessary to me. For example, the introduction of the __debug__ flag would not add much except another parameter. However, I like the idea about the adjust_dimensions function. It can be removed since it adds some computational and additional implicit operation on the user data, which might not be sufficient.

To support my conclusions, I added some benchmarks that show relative performance "as is" (time), with -O flag raised, with validate_input method removed, and both validate_input and adjust_dimensions methods removed.

method	time	time -O	time no validation	time no validation + no adjust
piq.tv	0.110	0.082	0.083	0.071
piq.psnr	0.175	0.086	0.083	0.077
piq.ssim	1.187	1.109	1.099	1.108
piq.ms_ssim	1.708	1.658	1.644	1.634
piq.LPIPS	28.941	30.202	30.146	30.093
piq.gmsd	0.483	0.391	0.389	0.398
piq.ms_gmsd	1.318	1.291	1.249	1.222
piq.mdsi	0.941	0.893	0.861	0.840
piq.haarpsi	1.187	1.161	1.113	1.091

Results above show that the most performance gain can be achieved by using the -O flag. Other changes do not significantly affect performance, especially when something more complicated than TV or PSNR is computed.

Regarding the reduction part, I do not see any reason to extract this part in some reusable function/method/module. Currently, we have these three lines of code repeated from time to time

return {'mean': score.mean,
            'sum': score.sum
            }[reduction](dim=0)

Only two of them can be extracted because we still need to leave the return statement. I can deal with two lines repeated from time to time in exchange for not introducing another abstraction.

To conclude:

1. adjust_dimensions can be removed
2. Input tensor shapes are already validated, so there is no need to introduce any other constraints
3. validate_input should be left "as is." None of the proposed options seem valuable enough to me
4. There is no need to introduce _reduce_output or something like that

[francois-rozet]

For example, the introduction of the __debug__ flag would not add much except another parameter.

__debug__ is a global variable builtin in Python. In fact, assert is a syntactic sugar over __debug__

assert cond, 'message'

is equivalent to

if __debug__:
    if not cond:
        raise AssertionError('message')

So I only propose to add if __debug__: return at the beginning of _validate_input so that it shortcuts the for and ifs since they won't do anything anyway if __debug__ is False.

---

Currently, we have these three lines of code repeated from time to time

return {'mean': score.mean,
            'sum': score.sum
            }[reduction](dim=0)

I believe you use 5 lines of code for the reduction.

    if reduction == 'none':
        return score
    return {'mean': score.mean,
            'sum': score.sum
            }[reduction](dim=0)

The problem is mainly maintainability: if you decide to add another reduction (prod maybe), you will need to modify every copy-pasted chunk of code. Also, creating a temporary dictionary at every execution, which is resolved directly, just to call a method, isn't what I would call good practice.

[zakajd]

Re-opened, because issue is important and we have at least some 🙃 consensus on what should be done.

I agree with @francois-rozet that reduction part could be improved by one-line function instead of a dict.

Will create PR in the morning, discussion can be continued there

[denproc]

Hi Guys!

1. Indeed, the validation should be optimised. I would be careful with assertions for input tensors as we perform validation for all functions with both single and multiple input tensors.
2. I agree with removing adjust_dimensions and strengthen validation condition for input tensors.
3. Regarding the wrapping of __debug__ into the _validate_input, for current version it can provide some performance gain. If we optimise the _validate_input first, the __debug__ introduction will provide even less improvements. Still, the change is just two lines of code, which can lead to some performance boost. It can be quite essential for the performance hungry user, who is desperate to use -O flag.
4. From the boosting performance discussion, the most important thing to refactor are exceptions, which are raised in such functions as fid.py, gmsd.py, haarpsi.py and others.
5. I would avoid new abstraction for reduction if we want to keep modularity of the code discussed previously.

[francois-rozet]

Hi, @zakajd, @denproc and @snk4tr. I see that, in the end, you did exactly what I had proposed 🙃. Good for you. However, could you mention that the code for _validate_input, _reduce and even the #Assertions section of the README is strongly inspired from my repository? I can also mention the Facebook disclaimer, the metrics table, the implementation of SSIM, and others. I had at least the decency to cite your repository in my README...