Qiskit-terra: Remove asserts from codebase

[atilag]

Welcome contributor!

Asserting is perfectly fine, there's nothing wrong with it, don't get me wrong :) ... but having said that, we do want to have a more grained control over our error control strategy. As an SDK we should have an homogeneous way of dealing with invalid states, so we want to change all:

assert(condition_is_met?)

... present in the code, with:

if condition_is_not_met:
    raise QISKitError('A meaningful error message goes here')

WARNING: Use the correct exception in every situation. We have different kind of exceptions for some of the modules, like: QasmError, SimulatorError, ResultError, ... all of them inherit from QISKitError.
We don't throw standard exceptions!!
If unsure, use QISKitError or ask me ;)

WARNING: The only part of the code where asserting is allowed (and encouraged) is on tests, so don't change asserts there :)

BTW,
Happy New Year!!

[waseem18]

Hi @atilag

I'm interested in working on this issue. I'll start working on this today and will put a WIP PR.

Thank You.

[atilag]

Great!
Looking forward for your PR :)

[waseem18]

@atilag
Any links related to developer documentation or to-know's to get to know the project code?

Interested in diving into project.

[pinakinathc]

@atilag since assert already has an error message, all that is required to be done is copy the error message into this new format using if condition?

[waseem18]

@pinakinathc If you're interested in working on this issue - you can start off. Because I haven't yet started looking into this issue and it might take some time for me to hop on this issue.

Thanks

[atilag]

@waseem18 I suggest you to start reading our CONTRIBUTING.md guide, but we don't have any specific doc about code organization. Anyway, for addressing this issue it's not needed to have a good understanding of the code :)

[atilag]

@pinakinathc Unfortunately, not all assert's have an error message. Figuring out a good error message so the user knows what's going on, it's probably the tough part.

[atilag]

So @waseem18 @pinakinathc ... this issue is a good candidate to split efforts, so you both can work on it and we can sync here to assign different files for each of you. What do you say! ... no need to fix them all, just a bunch of them ;)

[charlesastaylor]

Is anyone still working on this?

[1ucian0]

@charlesastaylor it seems that nobody is working on this. If you want to work on this, I can help you with your PR. Let me know and thanks!

[charlesastaylor]

I'll take a look over the next couple of days and get back to you, thanks!

[charlesastaylor]

@1ucian0 Just got round to having a look at this. Created a WIP PR

[charlesastaylor]

Also @1ucian0 @atilag I'm not quite sure which Exceptions I should be using. For the MapperError the docstring says "Base class for errors raised by mapper module." so I was fairly confident about using that one but for some of the other exceptions the docstring is less clear and not sure I should necessarily be blindly replacing all asserts in a module with the exception defined in said module. For example with the VisualizationError docstring - "For visualization specific errors." not really sure just because the assert is in the visualization module whether that exception is correct.

[1ucian0]

@charlesastaylor great you are taking over this! Let's discuss the exception types in the PR. Thanks!