april-tools / cirkit

a python framework to build, learn and reason about probabilistic circuits and tensor networks
https://cirkit-docs.readthedocs.io/en/latest/
GNU General Public License v3.0
71 stars 1 forks source link

Replace `assert` with `if`...`raise` #152

Open lkct opened 11 months ago

lkct commented 11 months ago

I suggest we replace assert statements with if...raise in the library code (cirkit/*), for the reasons:

  1. assert does not work in all cases. It assumes __debug__ and condition, which may be optimized out with different python config;
  2. AssertionError does not make sense. We should use a more specific exception for each case.

In unit tests we can keep assert as the above is not true. (1. Tests should be for debugging; 2. AssertionError indicates normal exit of library code with the result unexcepted.)

For benchmark and examples, where we use the library, we can keep assert because it's simpler, and we don't need the same level of robustness as the library itself.


Is it better to use the format of if not ...? In this way it's more obvious what predicate should hold true.

arranger1044 commented 11 months ago

I think asserts are ok, especially for now. And we want to have a __debug__ mode.

lkct commented 11 months ago

I think asserts are ok, especially for now

Yes, that's why I added "lo-pri" tag.

And we want to have a __debug__ mode

This is handled by python. In most cases we face, __debug__ is a constant True, but some people may want to deploy with it set to False. assert automatically conditions on __debug__ (and therefore is not an actual guard for release builds), but we can also use if __debug__: ... to do other things only during development. See also https://docs.python.org/3/library/constants.html?highlight=__debug__#debug__

lkct commented 10 months ago

Some notes about exceptions, expected to be useful in this repo



Currently in cirkit/new, the above is partially applied. But still a lot of assert has been used. Also TODO: make the error message better (e.g. use f-string to contain more info, instead of just a simple sentence,)

loreloc commented 4 months ago

In my opinion, it is ok to use asserts only for unrecoverable errors, i.e., errors for which the user is very unlikely to specify a catch implementation. For any other case we should use exceptions.