Skip to content
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

more Timer functionality #152

Merged
merged 5 commits into from Sep 25, 2020
Merged

more Timer functionality #152

merged 5 commits into from Sep 25, 2020

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Sep 25, 2020

This PR adds several features that I wanted when actually putting it into use in desispec:

  • silent option: record timing, but don't print log messages, e.g. to record timing for multiple MPI ranks without having every rank blasting timing log messages.
  • calculate_stats (like Ted's example from CMB-land suggested in the original Timer PR add Timer class to standardize algorithm timing reports #151): combine multiple Timers into a report with min/max/mean/median of start/stop/duration.
  • options to start and stop to set the time to use instead of time.time(), useful for recording timing of events that may have occurred before the timer was created, e.g. python startup and imports.
  • Timer.cancel to discard a named timer that had been started.

I'll submit a companion desispec PR that uses these features, but this PR will need to be reviewed and merged before that one can be finalized.

@sbailey
Copy link
Contributor Author

sbailey commented Sep 25, 2020

Travis tests are catching a timezone parsing issue that doesn't arise at NERSC or on my laptop. I'll investigate what the difference is, and back off on fancy date string parsing if needed. There might be some Travis churn as I sort this out.

@weaverba137
Copy link
Member

You're using pytz right?

@sbailey
Copy link
Contributor Author

sbailey commented Sep 25, 2020

No I wasn't using pytz (only vaguely aware of it...) Maybe for now I will just drop support for the fully generic Unix date output and require date +%s or ISO-8601 (which itself has lots of options...). I can return to adding vanilla date support if the pain of not having it exceeds the pain of implementing it.

@weaverba137
Copy link
Member

OK, ready for me to take a look?

@sbailey
Copy link
Contributor Author

sbailey commented Sep 25, 2020

Yes, please. Thanks.

Copy link
Member

@weaverba137 weaverba137 left a comment

Choose a reason for hiding this comment

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

Overall looks fine, but please note the comments. Most importantly, make sure self._print() is used consistently throughout the Timer class.

py/desiutil/test/test_timer.py Show resolved Hide resolved
py/desiutil/timer.py Outdated Show resolved Hide resolved
@weaverba137
Copy link
Member

PS, please update the change log.

@weaverba137
Copy link
Member

OK to merge assuming tests pass.

@sbailey sbailey merged commit 3cd5397 into master Sep 25, 2020
@sbailey sbailey deleted the timer branch September 25, 2020 23:16
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.

None yet

2 participants