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

pygmt.grdvolume: Refactor to store output in virtual files instead of temporary files #3102

Merged
merged 5 commits into from
Mar 15, 2024

Conversation

seisman
Copy link
Member

@seisman seisman commented Mar 13, 2024

Address #1318 and #2730.

Preview: https://pygmt-dev--3102.org.readthedocs.build/en/3102/api/generated/pygmt.grdvolume.html#pygmt.grdvolume

Benchmarks:

>>> from pygmt import grdvolume
>>> for i in [50, 25, 10, 5, 2, 1, 0.1]:
...     %timeit df = grdvolume("@earth_relief_01d_g", contour=[200, 400, i])
...

This branch:

12.7 ms ± 42 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
15.8 ms ± 78 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
25.1 ms ± 149 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
40.8 ms ± 244 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
87.2 ms ± 209 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
165 ms ± 227 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
1.57 s ± 4.11 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

Main branch:

10.3 ms ± 39.6 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
13.4 ms ± 23.3 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
22.4 ms ± 29.2 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
37.6 ms ± 43.2 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
83 ms ± 136 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
159 ms ± 225 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
1.52 s ± 1.12 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

This branch is slightly slower than the main branch, mainly because:

  1. this module is computational-intensive, so most of the time are spent in computation, rather than I/O
  2. For most real cases, the number of specified contours is small (e.g., 2000 for the last run), so using tempfile or virtualfile should have very close performance.

@seisman seisman added maintenance Boring but important stuff for the core devs needs review This PR has higher priority and needs review. run/benchmark Trigger the benchmark workflow in PRs labels Mar 13, 2024
@seisman seisman added this to the 0.12.0 milestone Mar 13, 2024
Copy link

codspeed-hq bot commented Mar 13, 2024

CodSpeed Performance Report

Merging #3102 will not alter performance

Comparing output/grdvolume (d24d75c) with main (3a507a8)

Summary

✅ 97 untouched benchmarks

🆕 1 new benchmarks
⁉️ 1 dropped benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main output/grdvolume Change
🆕 test_grdvolume N/A 294.7 ms N/A
⁉️ test_grdvolume_no_outgrid 41.7 ms N/A N/A

@seisman seisman changed the title pygmt.grdvolume: Improve performance by storing output in virtual files pygmt.grdvolume: Refactor to store output in virtual files instead of temporary files Mar 13, 2024
@seisman seisman added needs review This PR has higher priority and needs review. and removed needs review This PR has higher priority and needs review. labels Mar 13, 2024
@seisman seisman requested a review from a team March 14, 2024 13:11
@michaelgrund michaelgrund added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Mar 15, 2024
@seisman seisman removed final review call This PR requires final review and approval from a second reviewer run/benchmark Trigger the benchmark workflow in PRs labels Mar 15, 2024
@seisman seisman merged commit 4036322 into main Mar 15, 2024
13 of 15 checks passed
@seisman seisman deleted the output/grdvolume branch March 15, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants