Hello @lagru! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file benchmarks/benchmark_morphology.py:

Line 242:80: E501 line too long (87 > 79 characters)
Line 246:24: E231 missing whitespace after ','

Comment last updated at 2022-09-13 16:12:28 UTC

@lagru do you have a reference for how long these benchmarks take? I feel like the parameter matrix might be a bit too big. And I would argue that running local_minima on Hubble is unrealistic and not very relevant, while running it on the checkerboard is redundant.

You can run these locally quite easily. See the "asv run" help:

http://asv.readthedocs.io/en/latest/commands.html

You should probably limit yourself to a single environment from the benchmark matrix, as the envs take quite a while to make.

@jni Ha 😄, and here I expected that you'd have asked for benchmarks for local_minima if I had omitted them. My thought process was that local_minima using local_maxima under the hood is an implementation detail and not part of the API. Although I doubt that will change.

I haven't used local_maxima myself yet but using it without any kind of filtering on a natural image may not be "realistic" anyway. However I think these two extreme benchmarks cover a large part of local_maximas use cases quite well.

I'm happy to do a quick check on my machine although I doubt that my notebook is a good reference.

You should probably limit yourself to a single environment from the benchmark matrix, as the envs take quite a while to make.

It's a little bit faster if one uses conda to create them.

Codecov Report

Merging #3255 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3255   +/-   ##
=======================================
  Coverage   86.86%   86.86%           
=======================================
  Files         340      340           
  Lines       27484    27484           
=======================================
  Hits        23874    23874           
  Misses       3610     3610

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf3cf93...25a5e5f. Read the comment docs.

@jni On my machine (4 x Intel Core i5-4200U CPU @ 1.60GHz) running the 4 remaining benchmarks takes roughly 1 Minute and 20 seconds for 1 commit and 1 environment.

If you still want to reduce the parameter matrix we could remove the parameter "indices" which only really measures the difference induced by np.nonzero.

@lagru I would say that's about an order of magnitude too long. The only requirement is that it's long enough that we can get an accurate benchmark. So we should aim for an individual benchmark to run in ~1s, I think.

@jni With 33b8572 all 4 benchmarks together take ~20 s on my machine. Is that enough? Otherwise our only options are to remove the parameter "connectivity" or skip the 3D benchmark.

@lagru why not just reduce the size of the 3D image by reducing the sidelength of the cube, e.g. from (10, 10, 10) to (5, 5, 5)? That should give an 8x reduction in runtime, while testing the exact same principles?

@jni That has not much of an effect; the benchmarks take essentially the same time. The size of the array doesn't seem to be the factor here. Even if I use only really small arrays the 4 benchmarks take roughly 20 s. Of note is that these results are only true for the development version of asv (0.3.dev1274+a88114ce); for the most recent release 0.2.1 all 4 benchmarks take 45 seconds!

At this stage I really think that testing on my weak CPU may not be a good reference point. And I really want to avoid reducing the parameter matrix further because I think it's quite valuable to see the algorithms performance for different connectivity and border behavior.

Oh and I found a small bug: local_maxima([1, 1], allow_borders=False) should return array([0, 0]) but raises a ValueError in _offset_to_raveled_neighbors. Should I open an issue for this or just raise a PR when I find the time?

@lagru If you think that you can open a PR in a short term, then fine. Otherwise, you are invited to open an issue to do not forget. :)

@lagru Can you rebase this PR please? Otherwise, it looks good to me. @jni, do you agree after the last @lagru's comment?

Benchmark suite won't pass currently. See #4176 for a fix.

Hey @lagru!! 😉

Is this PR still relevant? The fourth task (out of four) is not relevant anymore; it may be either checked off or removed.

@mkcor, relevant in as much that more benchmarks don't hurt? I updated it and would say it's ready to go.

more benchmarks don't hurt

@lagru definitely! Since there hadn't been any activity in a while (:zzz: Dormant :wink:), I was wondering if maybe it was outdated. I guess this PR had simply fallen in the cracks! Should I wait for @jni's approval before merging? Or would you like to remove this review request?

I guess it had just fallen out of sight. I was wondering how you found this. 😄 As far as I'm aware I addressed all of @jni's feedback so I'm happy to get this merged.