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

Investigate large (>10%) deviations in benchmark of test_grdlandmask_no_outgrid #2942

Closed
weiji14 opened this issue Jan 2, 2024 · 4 comments · Fixed by #2945
Closed

Investigate large (>10%) deviations in benchmark of test_grdlandmask_no_outgrid #2942

weiji14 opened this issue Jan 2, 2024 · 4 comments · Fixed by #2945
Labels
maintenance Boring but important stuff for the core devs
Milestone

Comments

@weiji14
Copy link
Member

weiji14 commented Jan 2, 2024

Description of the problem

The test_grdlandmask_no_outgrid function's benchmark was added in #2911, but it seems like the performance has been deviating wildly (>10%) in many PRs since (e.g. #2937 (comment), #2938 (comment)), even though there has been no evident change in the code of grdlandmask or any of the underlying clib functions.

See tracked performance at https://codspeed.io/GenericMappingTools/pygmt/benchmarks/pygmt/tests/test_grdlandmask.py::test_grdlandmask_no_outgrid

Sample flame graph from https://codspeed.io/GenericMappingTools/pygmt/branches/windows-multiprocessing:

image

Opening this issue to discuss why the variance might be so high, and if there are ways to mitigate this.

Minimal Complete Verifiable Example

make test PYTEST_EXTRA="-r P --pyargs pygmt -k test_grdlandmask_no_outgrid --codspeed"

Full error message

No response

System information

PyGMT information:
    version: v0.10.1.dev187+g322f8889
  System information:
    python: 3.12.1 | packaged by conda-forge | (main, Dec 23 2023, 08:03:24) [GCC 12.3.0]
    executable: /usr/share/miniconda/bin/python
    machine: Linux-6.2.0-1018-azure-x86_64-with-glibc2.35
  Dependency information:
    numpy: 1.26.2
    pandas: 2.1.4
    xarray: 2023.12.0
    netCDF4: 1.6.5
    packaging: 23.2
    contextily: None
    geopandas: 0.14.1
    ipython: None
    rioxarray: None
    ghostscript: 10.02.1
  GMT library information:
    binary version: 6.4.0
    cores: 4
    grid layout: rows
    image layout: 
    library path: /usr/share/miniconda/lib/libgmt.so
    padding: 2
    plugin dir: /usr/share/miniconda/lib/gmt/plugins
    share dir: /usr/share/miniconda/share/gmt
    version: 6.4.0
@weiji14 weiji14 added the triage Unsure where this issue fits label Jan 2, 2024
@weiji14
Copy link
Member Author

weiji14 commented Jan 2, 2024

@seisman
Copy link
Member

seisman commented Jan 2, 2024

One hypothesis is that the high variance may be due to multithreading with OpenMP? These GMT functions support the cores/-x parameter

Sounds reasonable.

@weiji14
Copy link
Member Author

weiji14 commented Jan 2, 2024

Seems that grdlandmask doesn't have the cores (-x) alias yet, so I've opened #2944 first to set the alias (and also see if setting a fixed cores=2 limit works). Will handle grdsample, sph2grd, etc separately in #2945.

@seisman seisman reopened this Jan 4, 2024
@seisman seisman added maintenance Boring but important stuff for the core devs and removed triage Unsure where this issue fits labels Jan 6, 2024
@seisman seisman added this to the 0.11.0 milestone Jan 6, 2024
@seisman
Copy link
Member

seisman commented Jan 6, 2024

The test_grdlandmask_no_outgrid test has a relatively stable performance since the fix in PR #2944:

https://codspeed.io/GenericMappingTools/pygmt/benchmarks/pygmt/tests/test_grdlandmask.py::test_grdlandmask_no_outgrid

Closing this issue.

@seisman seisman closed this as completed Jan 6, 2024
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 a pull request may close this issue.

2 participants