Skip to content

Reduce time-tracking overhead - #1988

Merged
dweindl merged 8 commits into
developfrom
test_clock
Feb 14, 2023
Merged

Reduce time-tracking overhead#1988
dweindl merged 8 commits into
developfrom
test_clock

Conversation

@dweindl

@dweindl dweindl commented Feb 13, 2023

Copy link
Copy Markdown
Member
  • Don't check clock() on every rhs evaluation, but only every 500.
  • Avoid clock() completely, if there is no max time specified.

@codecov

codecov Bot commented Feb 13, 2023

Copy link
Copy Markdown

Codecov Report

Merging #1988 (066dc22) into develop (753126a) will increase coverage by 0.15%.
The diff coverage is 92.30%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1988      +/-   ##
===========================================
+ Coverage    75.92%   76.08%   +0.15%     
===========================================
  Files           76       76              
  Lines        12995    13002       +7     
===========================================
+ Hits          9867     9893      +26     
+ Misses        3128     3109      -19     
Flag Coverage Δ
cpp 73.22% <92.30%> (+0.12%) ⬆️
petab 60.05% <ø> (ø)
python 68.89% <ø> (+0.22%) ⬆️
sbmlsuite ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/solver_idas.cpp 37.70% <50.00%> (ø)
include/amici/solver.h 83.78% <100.00%> (+0.45%) ⬆️
src/solver.cpp 76.86% <100.00%> (+0.17%) ⬆️
src/solver_cvodes.cpp 70.06% <100.00%> (ø)
src/exception.cpp 74.28% <0.00%> (ø)
src/sundials_matrix_wrapper.cpp 81.02% <0.00%> (+0.81%) ⬆️
...thon/sdist/amici/conserved_quantities_demartino.py 65.55% <0.00%> (+1.95%) ⬆️
src/amici.cpp 76.00% <0.00%> (+4.00%) ⬆️

* Don't check `clock()` on every rhs evaluation, but only every 1000.
* Avoid `clock()` completely, if there is no max time specified.
@dweindl dweindl changed the title Reduce clock() calls Reduce time-tracking overhead Feb 13, 2023
@dweindl
dweindl marked this pull request as ready for review February 13, 2023 14:32
@dweindl
dweindl requested a review from a team as a code owner February 13, 2023 14:32
Comment thread src/solver.cpp Outdated
Co-authored-by: Fabian Fröhlich <fabian_froehlich@hms.harvard.edu>
Comment thread src/solver_cvodes.cpp Outdated
// Avoid expensive syscalls by checking only every N rhs evaluations
static int eval_counter = 0;
int const interval = 1000;
if (++eval_counter / interval) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about rather extending solver->timeExceeded() by adding the counter as private member of solver and checking against that in every call?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thought about it, but it wouldn't work with

if(ex.error_code == AMICI_RHSFUNC_FAIL && solver.timeExceeded()) {

There we want to check on every call. Would have to add some force_check=True argument or a separate method.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thought about it, but it wouldn't work with

if(ex.error_code == AMICI_RHSFUNC_FAIL && solver.timeExceeded()) {

There we want to check on every call. Would have to add some force_check=True argument or a separate method.

I think I would prefer that.

@dweindl
dweindl requested a review from FFroehlich February 14, 2023 11:54
@sonarqubecloud

Copy link
Copy Markdown

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@FFroehlich FFroehlich left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@dweindl
dweindl merged commit 529320f into develop Feb 14, 2023
@dweindl
dweindl deleted the test_clock branch February 14, 2023 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants