-
-
Notifications
You must be signed in to change notification settings - Fork 486
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
add unit benchmarks #2092
add unit benchmarks #2092
Conversation
benchmarks/unit_benchmarks.py
Outdated
|
||
def time_parameterise(): | ||
|
||
global param |
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.
you should be able to do this without using global
. I have never seen a good reason to use it
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.
The benchmarks don't work without these global variables even after adding the setup functions. I don't know why this is the case, but when I run the benchmarks with the global variables they work
benchmarks/unit_benchmarks.py
Outdated
R = 1e-5 | ||
|
||
|
||
def time_create_expression(): |
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.
You should define a class like with the other benchmarks. The first bit can be pushed under the setup
method, and then have the different benchmarks.
benchmarks/unit_benchmarks.py
Outdated
time_discretise.setup = setup_discretise | ||
|
||
|
||
def setup_solve(): |
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.
The discretisation and parameterisation methods modify the model, so if you define model
to be a variable of the class then hopefully you do not need to initialise every single 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.
I tried this but got an error because asv
runs every method separately, so the changes made in one method don't carry on to others. For example-
for the time_solve
benchmark to work we will need to run previous methods in its setup, and a class cannot have more than 1 setup; hence, I cannot put this under the class containing other time_*
functions as they require a different setup
Codecov Report
@@ Coverage Diff @@
## develop #2092 +/- ##
===========================================
- Coverage 99.38% 99.38% -0.01%
===========================================
Files 348 348
Lines 19255 19234 -21
===========================================
- Hits 19136 19115 -21
Misses 119 119
Continue to review full report at Codecov.
|
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.
Thanks, this looks much cleaner!
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.
Looks great, thanks Vaibhav!
Description
Created unit benchmarks (create an expression, parameterise a model, discretise a model, and solve a model) using this example.
I tried developing these benchmarks using classes and inheritance, but
asv
runstime_*
functions of the parent class in the child classes too. This results in benchmarks running multiple times.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:
$ flake8
$ python run-tests.py --unit
$ cd docs
and then$ make clean; make html
You can run all three at once, using
$ python run-tests.py --quick
.Further checks: