Skip to content
This repository has been archived by the owner on Oct 14, 2023. It is now read-only.

Add error messages to assert conditions #1367

Merged
merged 4 commits into from Oct 30, 2021
Merged

Conversation

Yash-10
Copy link
Member

@Yash-10 Yash-10 commented Oct 28, 2021

Trying to resolve #1294 :)

@codecov
Copy link

codecov bot commented Oct 28, 2021

Codecov Report

Merging #1367 (d65ed56) into main (18a390d) will increase coverage by 0.10%.
The diff coverage is 63.63%.

❗ Current head d65ed56 differs from pull request most recent head 782602a. Consider uploading reports for the commit 782602a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1367      +/-   ##
==========================================
+ Coverage   92.37%   92.47%   +0.10%     
==========================================
  Files          82       82              
  Lines        4273     4277       +4     
  Branches      357      360       +3     
==========================================
+ Hits         3947     3955       +8     
+ Misses        246      238       -8     
- Partials       80       84       +4     
Impacted Files Coverage Δ
src/poliastro/frames/fixed.py 91.89% <0.00%> (-7.20%) ⬇️
src/poliastro/core/iod.py 91.16% <100.00%> (ø)
src/poliastro/twobody/orbit.py 87.90% <100.00%> (+2.86%) ⬆️
src/poliastro/twobody/propagation.py 91.75% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18a390d...782602a. Read the comment docs.

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Yash-10 , sorry but I don't think these errors are very useful 😕 The reason is that users will see something like this:

<ipython-input-1-a9d94d6ee2f1> in f(x)
      1 def f(x):
----> 2     assert x > 0, "x must be greater than zero"
      3     return x
      4 

AssertionError: x must be greater than zero

But they won't know what the actual value of the offending variables was!

We cannot have dynamic assertions though, because numba doesn't support it:

In [10]: @njit
    ...: def f(x):
    ...:     assert x > 0, str(x)
    ...:     return x
    ...: 

In [11]: f(1.0)
---------------------------------------------------------------------------
ConstantInferenceError                    Traceback (most recent call last)
<ipython-input-11-8afb63b2369d> in <module>
----> 1 f(1.0)

~/Projects/LSF/poliastro/library/.venv/lib/python3.9/site-packages/numba/core/dispatcher.py in _compile_for_args(self, *args, **kws)
    430             # this is from trying to infer something as constant when it isn't
    431             # or isn't supported as a constant
--> 432             error_rewrite(e, 'constant_inference')
    433         except Exception as e:
    434             if config.SHOW_HELP:

~/Projects/LSF/poliastro/library/.venv/lib/python3.9/site-packages/numba/core/dispatcher.py in error_rewrite(e, issue_type)
    359                 raise e
    360             else:
--> 361                 raise e.with_traceback(None)
    362 
    363         argtypes = []

ConstantInferenceError: Failed in nopython mode pipeline (step: nopython rewrites)
Constant inference not possible for: call $12load_global.1(x, func=$12load_global.1, args=[Var(x, <ipython-input-10-4db6ef733143>:3)], kws=(), vararg=None)

File "<ipython-input-10-4db6ef733143>", line 3:
def f(x):
    assert x > 0, str(x)
    ^


so, I don't know what the solution would be here.

I noticed that you did other cleanups apart from the assertions, are they still useful?

Remove assert message

Remove
@Yash-10
Copy link
Member Author

Yash-10 commented Oct 29, 2021

I noticed that you did other cleanups apart from the assertions, are they still useful?

Yeah, it seems so! As discussed, I removed assert error messages for core methods, and kept them for non-core methods, along with small cleanups 👍

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of comments!

src/poliastro/frames/fixed.py Outdated Show resolved Hide resolved
src/poliastro/twobody/propagation.py Outdated Show resolved Hide resolved
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks perfect now, thanks @Yash-10 !

@Yash-10
Copy link
Member Author

Yash-10 commented Oct 30, 2021

It looks like auto-merge couldn't merge this because of codecov/patch, right?

@astrojuanlu astrojuanlu merged commit 95cf3ff into poliastro:main Oct 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Converting assert statements to separate unit tests?
2 participants