-
-
Notifications
You must be signed in to change notification settings - Fork 606
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Initial attempt at implementing property-based testing using Hypothesis #4724
Conversation
@RohitP2005 Add Here: Line 100 in 421784e
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added reviews for test_averages.py
. I'll add reviews for test_unary_operators.py
once the failing tests are fixed.
@@ -267,8 +271,9 @@ def test_r_average(self): | |||
assert pybamm.r_average(a + b) == pybamm.r_average(a) + pybamm.r_average(b) | |||
assert pybamm.r_average(a - b) == pybamm.r_average(a) - pybamm.r_average(b) | |||
|
|||
def test_yz_average(self): | |||
a = pybamm.Scalar(1) | |||
@given(st.integers(min_value=-(2**63), max_value=2**63 - 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explained here at #4724 (comment)
Also, fix the PR heading. It's not proper. |
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
@prady0t |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4724 +/- ##
========================================
Coverage 98.70% 98.70%
========================================
Files 304 304
Lines 23452 23483 +31
========================================
+ Hits 23149 23180 +31
Misses 303 303 ☔ View full report in Codecov by Sentry. |
@@ -174,9 +177,10 @@ def test_x_average(self): | |||
assert pybamm.x_average(a + b) == pybamm.x_average(a) + pybamm.x_average(b) | |||
assert pybamm.x_average(a - b) == pybamm.x_average(a) - pybamm.x_average(b) | |||
|
|||
def test_size_average(self): | |||
@given(st.integers(min_value=-(2**63), max_value=2**63 - 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Having large-sized integers causes buffer overflow (probably because of different ways int is handled by pybamm and numpy ). Yes it's good to restrict the range here.
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Co-authored-by: Pradyot Ranjan <99216956+prady0t@users.noreply.github.com>
Hey @agriyakhetarpal can u trigger the CI workflows for this branch |
@RohitP2005 Triggered CI and updated the branch |
Requesting a review from @prady0t on this. |
thanks @kratman |
de2699b
to
f191876
Compare
Hey @agriyakhetarpal I need updates on this PR. Its been there for a long time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Sorry for the delay in the review, @RohitP2005! Thanks for your work here. I just have one comment – happy to approve after that.
Hi @prady0t, I plan to merge this early next week so that @RohitP2005 can proceed further – unless you have comments by then. |
@agriyakhetarpal I would like a final check by @prady0t |
I think I would like to see this be limited to only where it is needed and useful rather than all tests. We would not want to cause the complexity and runtime of our tests to explode from excessive testing. The functions you have done so far are fairly straight forward, but tests in other parts of the code can be quite slow and complicated. My other concern is that the codebase is physics based, so you need to make sure that the value ranges make sense for the codebase as well. Some parameters may only be valid in the 0-1 range or need to be only positive floats. |
Other than the above review, everything look good. Thanks @RohitP2005 |
Description
This change introduces the use of Hypothesis-based testing in two files and is being applied to selective functions as requested by the author and members of the issue #4703 (comment)
Starts #4703
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python -m pytest
(or$ nox -s tests
)$ python -m pytest --doctest-plus src
(or$ nox -s doctests
)You can run integration tests, unit tests, and doctests together at once, using
$ nox -s quick
.Further checks: