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

Function profiling should use time.time_ns instead of time.time #1246

Closed
michaelosthege opened this issue Oct 9, 2022 · 3 comments · Fixed by #1262
Closed

Function profiling should use time.time_ns instead of time.time #1246

michaelosthege opened this issue Oct 9, 2022 · 3 comments · Fixed by #1262
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed refactor This issue involves refactoring

Comments

@michaelosthege
Copy link
Contributor

Description of your problem or feature request

The function profiling in

t0 = time.time()
(and more lines that file) uses time.time() which has varying precision on different platforms.

time.time_ns() or time.time_ns() / (10 ** 9) for float seconds should be used for high-precision performance measurement.

Further reading: https://stackoverflow.com/a/1938096

Versions and main components

  • Aesara version: 2.8.7
  • Operating system: mac-OS, Windows
ricardoV94 added a commit to ricardoV94/pymc that referenced this issue Oct 9, 2022
@brandonwillard brandonwillard added enhancement New feature or request refactor This issue involves refactoring labels Oct 9, 2022
@brandonwillard brandonwillard added this to the Clean up after Theano milestone Oct 9, 2022
@brandonwillard brandonwillard added help wanted Extra attention is needed good first issue Good for newcomers labels Oct 9, 2022
michaelosthege pushed a commit to pymc-devs/pymc that referenced this issue Oct 9, 2022
@brandonwillard
Copy link
Member

brandonwillard commented Oct 18, 2022

Isn't time.perf_counter[_ns] provided explicitly for these purposes?

@anirudhacharya
Copy link
Contributor

anirudhacharya commented Oct 18, 2022

@brandonwillard yes, since we are only using it to measure time difference and nowhere are we logging the actual time of an event, time.perf_counter() makes more sense than time.time().

Depending on how we want to do the profiling, the options are -

  1. time.time() / time.time_ns()
  2. time.perf_counter() - can be used to record the difference in clock time.
  3. time.process_time() - can be used to record the difference in process time.
import time

def perf_count():
    tic = time.perf_counter()
    time.sleep(1)
    toc = time.perf_counter()
    return toc - tic

def proc_time():
    tic = time.process_time()
    time.sleep(1)
    toc = time.process_time()
    return toc - tic

print(perf_count())
print(proc_time())
Output 
-------
1.003998823
5.9000000000003494e-05

Should I go ahead and change it to perf_counter?

@brandonwillard
Copy link
Member

Should I go ahead and change it to perf_counter?

Yes, that sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed refactor This issue involves refactoring
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants