Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace assert with if...raise #152

Open
lkct opened this issue Nov 17, 2023 · 4 comments
Open

Replace assert with if...raise #152

lkct opened this issue Nov 17, 2023 · 4 comments
Labels
enhancement New feature or request low-priority Low priority issues refactor Improve code style and readability user-facing Design mainly for users but not devs

Comments

@lkct
Copy link
Member

lkct commented Nov 17, 2023

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.

@lkct lkct added enhancement New feature or request low-priority Low priority issues labels Nov 17, 2023
@arranger1044
Copy link
Member

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

@lkct
Copy link
Member Author

lkct commented Nov 17, 2023

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
Copy link
Member Author

lkct commented Dec 13, 2023

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

  • The operation is not defined at all for the given type/class:
    • TypeError.
  • The operation is defined for the correct type, but not for the specific value given:
    • ValueError;
    • or AttributeError, when __{get|set}attr__ fails;
    • or IndexError|KeyError, when __{get|set}item__ fails for some index/key.
  • The operation can be defined, but not yet in the code:
    • NotImplementedError
  • Whatever else:
    • AssertionError for assertion (haven't tried to be fitted in the above);
    • RuntimeError for anything that cannot fit in any above.

  • assert: a guard that only helps debugging/linting, should never be hit during runtime.

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,)

@lkct lkct added refactor Improve code style and readability user-facing Design mainly for users but not devs labels Feb 22, 2024
@loreloc
Copy link
Member

loreloc commented Jun 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request low-priority Low priority issues refactor Improve code style and readability user-facing Design mainly for users but not devs
Projects
None yet
Development

No branches or pull requests

3 participants