remove assert in code

[Can-Zhao]

There are several assert in code, which can be dangerous, e.g.

GenerativeModels/generative/networks/nets/diffusion_model_unet.py

Line 471 in ad973b8

assert len(timesteps.shape) == 1, "Timesteps should be a 1d-array"

GenerativeModels/generative/networks/nets/diffusion_model_unet.py

Line 562 in ad973b8

assert x.shape[1] == self.num_channels

assert should be used only for debugging. It will be ignored when running in a product environment. We should remove all the assert in source code, except for testing script.

[ericspod]

Both these uses should be replaced with checks which raise ValueError if false. Asserts in general though are correct to leave in production code and can be disabled at runtime. The purpose of asserts is to check that some implicit condition is true, and to document these in case they aren't obvious from the code. This could be done as a comment but an assert makes it explicit and is a little more rigorous in the design-by-contract direction.

[Can-Zhao]

Both these uses should be replaced with checks which raise ValueError if false. Asserts in general though are correct to leave in production code and can be disabled at runtime. The purpose of asserts is to check that some implicit condition is true, and to document these in case they aren't obvious from the code. This could be done as a comment but an assert makes it explicit and is a little more rigorous in the design-by-contract direction.

Thank you! But at least code with assert cannot be converted to torchscript...Assert statements are being disabled/skipped by Python if the code is run in optimized mode (release setting). Why not just avoid such risk?

[ericspod]

Thank you! But at least code with assert cannot be converted to torchscript...Assert statements are being disabled/skipped by Python if the code is run in optimized mode (release setting). Why not just avoid such risk?

The asserts are checking that inputs meet the contract for the method, this is something for public functions/methods you don't want disabled in production code. This is a design principle I would suggest in general for most code but if we have specific technical requirements like Torchscript then can omit it.

[Can-Zhao]

Thank you! But at least code with assert cannot be converted to torchscript...Assert statements are being disabled/skipped by Python if the code is run in optimized mode (release setting). Why not just avoid such risk?

The asserts are checking that inputs meet the contract for the method, this is something for public functions/methods you don't want disabled in production code. This is a design principle I would suggest in general for most code but if we have specific technical requirements like Torchscript then can omit it.

Thanks. Then maybe let’s change assert to if/else for all the network models. Then we do not need to worry about torchscript for the models. Would that work? Thank you!

[Can-Zhao]

Thank you! But at least code with assert cannot be converted to torchscript...Assert statements are being disabled/skipped by Python if the code is run in optimized mode (release setting). Why not just avoid such risk?

The asserts are checking that inputs meet the contract for the method, this is something for public functions/methods you don't want disabled in production code. This is a design principle I would suggest in general for most code but if we have specific technical requirements like Torchscript then can omit it.

And sorry I might not explain it well. Using assert is better than doing nothing. But it still has some risk. Using if/else and ValueError is better. Thank you for your patience.

[ericspod]

Where we can yes I'd say assert that is checking for inputs should be if-ValueError checks, unless this break Torchscript compatibility (which should only be in forward).

I'd like to push a little in the design-by-contract direction but it's too much to be implementing that though! I'd always say that checking inputs is important but that is currently done with if-ValueError checks and not assert because it can be disabled. The proper use of assert is to make statements of truth mid-algorithm that aren't obvious from the code. It should assert some property that should always be true unless the algorithm is incorrect, so this is as much a test as a piece of documentation. Checking this at production shouldn't be needed since we assume the algorithm is correct by production time, but because inputs are dependent on external sources they need to be checked always I would say. Thanks!

[Can-Zhao]

let’s change assert to if/else for all the network models

Yes, using assert is more convenient! Thank you for clarifying it. How about changing assertion to if-ValueError for all the network architecture like autoencoder or diffusion network? So we do not need to worry about torchscript.

[ericspod]

How about changing assertion to if-ValueError for all the network architecture like autoencoder or diffusion network? So we do not need to worry about torchscript.

We should do that for those asserts being used to check input values only. If they've been used elsewhere for other things we should look at what that is or leave them. I do know we've been a bit inconsistent in core and elsewhere so it would be a good idea to have a look over things.

[Can-Zhao]

How about changing assertion to if-ValueError for all the network architecture like autoencoder or diffusion network? So we do not need to worry about torchscript.

We should do that for those asserts being used to check input values only. If they've been used elsewhere for other things we should look at what that is or leave them. I do know we've been a bit inconsistent in core and elsewhere so it would be a good idea to have a look over things.

Thank you! As long as we keep in mind that assertion will be skipped in product, then it is fine. Thank you for the discussion.