alan-turing-institute / deepsensor

A Python package for tackling diverse environmental prediction tasks with NPs.
https://alan-turing-institute.github.io/deepsensor/
MIT License
94 stars 16 forks source link

Use of `assert` #112

Closed davidwilby closed 4 months ago

davidwilby commented 7 months ago

Mainly just keeping this here as a note to myself to potentially address in future.

I noticed that assert is used (understandably) throughout deepsensor for checking values, sizes etc.

As far as I understand, assert in "production" code isn't advised. This is mainly because when run in optimized mode python -0 <script.py> (or via some other means), the internal __debug__ variable is set to False and stops respecting assert statements. (See official docs: https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement)

assert is essentially:

if __debug__:
    if not expression: raise AssertionError

Now, this is potentially a minor case, but if used in a production setting, it's possible that deepsensor could be used with python's optimized mode and fall over.

I'm in two minds about this because assert statements are concise, but potentially hazardous in the long-run.

Instead of assert, errors should be handled explicitly with raise (as far as I understand it).

tom-andersson commented 6 months ago

Thanks for raising @davidwilby, interesting, I wasn't aware of this! I'm also in two minds about the readability sacrifice, but if you feel strongly feel free to submit a PR.

As you suggest though, this probably isn't a top priority until DeepSensor is likely to be used in a production setting.

tom-andersson commented 4 months ago

Closing because I don't foresee any need for using DeepSensor in optimised mode in the near-medium future