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

Performance Shift(s): ca42c30b #5168

Closed
github-actions bot opened this issue Feb 21, 2023 · 3 comments
Closed

Performance Shift(s): ca42c30b #5168

github-actions bot opened this issue Feb 21, 2023 · 3 comments
Assignees
Labels
Bot A bot generated issue/pull-request Type: Performance

Comments

@github-actions
Copy link
Contributor

Benchmark comparison has identified performance shifts at

Please review the report below and take corrective/congratulatory action as appropriate 🙂

Performance shift report
       before           after         ratio
     [504c188f]       [ca42c30b]
     <main~2>         <main~1>  
+        407±20μs         517±20μs     1.27  coords.AuxCoordLazy.time_bounds
+        391±20μs         510±20μs     1.30  coords.AuxCoordLazy.time_points
+        411±20μs         520±30μs     1.26  experimental.ugrid.ConnectivityLazy.time_indices(1000000)
+        409±20μs         515±20μs     1.26  experimental.ugrid.ConnectivityLazy.time_indices(6)

Generated by GHA run 4227658143

@github-actions github-actions bot added Bot A bot generated issue/pull-request Type: Performance labels Feb 21, 2023
@trexfeathers
Copy link
Contributor

Presumably linked to the new Dask version? Are we OK with this? Less than 1 millisecond, even for highly scaled examples (e.g. time_indices(1000000)).

@lbdreyer
Copy link
Member

lbdreyer commented Feb 21, 2023

Presumably linked to the new Dask version? Are we OK with this? Less than 1 millisecond, even for highly scaled examples (e.g. time_indices(1000000)).

It is interesting that it doesn't scale with the number of points. It implies that perhaps it is something to do with the setting up of the lazy array (with dask as you say) and the only reason these benchmarks caught it is because they are so fast?

If that were the case it seems like this change is very minimal and not to be worried about

@bjlittle bjlittle closed this as not planned Won't fix, can't repro, duplicate, stale Feb 22, 2023
@pp-mo
Copy link
Member

pp-mo commented Feb 22, 2023

@lbdreyer doesn't scale with the number of points.

From my reading, it is not actually actually reading any data since the test example is made from a numpy array.
So it is like da.from_array(np.array(...)).compute(), which simply returns the underlying array object. No copying or moving data is involved.

>>> import numpy as np
>>> import dask.array as da
>>> np1 = np.arange(24.).reshape((4,3,2))
>>> da1 = da.from_array(np1)
>>> da1.compute() is np1
True
>>> 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bot A bot generated issue/pull-request Type: Performance
Projects
None yet
Development

No branches or pull requests

4 participants