Feature timing, Closes #524 #699
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #699 +/- ##
==========================================
Coverage ? 73.94%
==========================================
Files ? 50
Lines ? 7434
Branches ? 0
==========================================
Hits ? 5497
Misses ? 1937
Partials ? 0
Continue to review full report at Codecov.
|
|
Had a brief look at the code. Please make the cpu times private I am currently in the process of refactoring the whole handling of rdata and the goal is no never have solver/model interact with rdata, which increases modularity/interoperability of these classes and adds some additional flexibility in terms of when and whether data is written to rdata. |
|
And please add |
src/solver.cpp
Outdated
| @@ -54,7 +54,7 @@ int Solver::run(const realtype tout, ReturnData *rdata) const { | |||
| } else { | |||
| status = solve(tout, AMICI_NORMAL); | |||
| } | |||
| rdata->cpu_time +=(double)((clock() - starttime) * 1000) / CLOCKS_PER_SEC; | |||
| incrementCpuTime((realtype)((clock() - starttime) * 1000) / CLOCKS_PER_SEC); | |||
There was a problem hiding this comment.
its a private member, you should still be able to modify it without a setter routine. I don't think we want a setter routine as the Solver class itself should be the only one that modifies those values.
There was a problem hiding this comment.
okay, I'll do that. Thanks!
Other question: How about the way, it's currently implemented in forward or backwardproblem.cpp? Would you prefer incrementing the rdata-member after every run, or once at the very end of the routine (as it is now)?
There was a problem hiding this comment.
Other question: How about the way, it's currently implemented in forward or backwardproblem.cpp? Would you prefer incrementing the rdata-member after every run, or once at the very end of the routine (as it is now)?
It's fine as it is now, as it is consistent with how other things are handled at the moment. In the long run I want to change this to be all done in a single function that can be called independently of forward/backwardproblem, but one thing at a time.
|
@FFroehlich : As I'm (hopefully) done so far but have no permission for merging: Can you please merge, once the checks have passed? |
I don't see why you wouldn't have permission, we should fix that. |
At least, for the last PRs I didn't have it... So I guess I don't have it now... |
Added cputime for forward and backward solve to the output of Amici, to get a feeling for the overhead from python to C++, and to see how the pure computation time scales with the size of the model (for comparison: theory vs. practice).