Report CPU Time alongside Wall Time#275
Merged
zerowidth merged 4 commits intogithub:mainfrom Dec 16, 2024
Merged
Conversation
We'd like to be able to quantify CPU time consumed while executing the candidate and control blocks during science experiments. Changes include: - Added `cpu_time` attribute to `Scientist::Observation`. - Refactored initialization to capture both start/end times for wall time and CPU time. - Updated tests to include CPU-intensive work so we can verify we're recording CPU time correctly. This allows us to track cpu time more precisely when making improvements.
madelineleclair
left a comment
madelineleclair
left a comment
There was a problem hiding this comment.
Looks good from github/feature-management-reviewers
zerowidth
reviewed
Dec 11, 2024
This supports both the old version (single value) and new version (hash-based). I've added back the tests from the old version to check that the code stills works when we provide a single value. I've also adapted the README a bit to explain both versions.
zerowidth
approved these changes
Dec 13, 2024
Contributor
zerowidth
left a comment
zerowidth
left a comment
There was a problem hiding this comment.
Looks good! Question about the doc, I could go either way, what do you think?
README.md
Outdated
| If you're writing tests that depend on specific timing values, you can provide canned durations using the `fabricate_durations_for_testing_purposes` method, and Scientist will report these in `Scientist::Observation#duration` and `Scientist::Observation#cpu_time` instead of the actual execution times. | ||
| If you're writing tests that depend on specific timing values, you can provide canned durations using the `fabricate_durations_for_testing_purposes` method. This can be done using either the old version (single value) or the new version (hash-based) to include both duration and cpu_time. | ||
|
|
||
| ##### Old version (Single Value) |
Contributor
There was a problem hiding this comment.
I'd be fine leaving this section of the doc out -- the API has changed but the old version still works, but we may as well encourage the updated version
Contributor
Author
There was a problem hiding this comment.
Hokey doke! I've removed the "Old version" section and rephrased.
zerowidth
approved these changes
Dec 16, 2024
Contributor
Author
|
Thanks for your help @zerowidth 🙇♀️ ! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We'd like to be able to quantify CPU time consumed while executing the
candidate and control blocks during science experiments.
Changes include:
cpu_timeattribute toScientist::Observation.time and CPU time.
recording CPU time correctly.
This allows us to track cpu time more precisely when making improvements.