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

builtins: Add devito version of NumPy's count_nonzero, nonzero #1815

Closed
wants to merge 2 commits into from

Conversation

georgebisbas
Copy link
Contributor

@georgebisbas georgebisbas commented Dec 24, 2021

Add a Devito version of NumPy's count_nonzero, nonzero:
https://numpy.org/doc/stable/reference/generated/numpy.count_nonzero.html
This PR is needed towards implementing steps 1,2 of p4 in https://arxiv.org/pdf/2010.10248.pdf.
In the paper, I was using NumPy, but now to automate through Devito, I would like to generate it through Devito.
Numpy's version is faster unless we change to collapse(1). Then Devito is faster for like int32 (20000, 20000) sized functions.
If reviewers require more benchmarking, happy to do so.

Generated code is like:

  /* Begin section0 */
  START_TIMER(section0)
  #pragma omp parallel num_threads(nthreads)
  {
    #pragma omp for collapse(2) schedule(static,1) reduction(+:h1[1])
    for (int x = x_m; x <= x_M; x += 1)
    {
      for (int y = y_m; y <= y_M; y += 1)
      {
        if (f[x + 1][y + 1] != 0)
        {
          int r0 = 1;
          h1[1] += r0;
        }
      }
    }
  }
  STOP_TIMER(section0,timers)
  /* End section0 */

  return 0;

@georgebisbas georgebisbas added the API api (symbolics, types, ...) label Dec 24, 2021
@georgebisbas georgebisbas self-assigned this Dec 24, 2021
@codecov
Copy link

codecov bot commented Dec 24, 2021

Codecov Report

Merging #1815 (87695b4) into master (6abd441) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1815      +/-   ##
==========================================
- Coverage   89.56%   89.54%   -0.02%     
==========================================
  Files         209      209              
  Lines       34381    34440      +59     
  Branches     5198     5201       +3     
==========================================
+ Hits        30793    30841      +48     
- Misses       3098     3107       +9     
- Partials      490      492       +2     
Impacted Files Coverage Δ
devito/builtins/arithmetic.py 86.25% <100.00%> (+6.25%) ⬆️
tests/test_builtins.py 100.00% <100.00%> (ø)
tests/test_dle.py 96.94% <0.00%> (-0.95%) ⬇️
devito/types/basic.py 94.94% <0.00%> (-0.75%) ⬇️
devito/operator/operator.py 90.68% <0.00%> (-0.23%) ⬇️
tests/test_dse.py 99.72% <0.00%> (-0.07%) ⬇️

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 6abd441...87695b4. Read the comment docs.

@georgebisbas georgebisbas force-pushed the devito_nonzero branch 3 times, most recently from 00f0294 to eb752c9 Compare December 27, 2021 14:56
Copy link
Contributor

@FabioLuporini FabioLuporini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Numpy's version is faster unless we change to collapse(1)

you may use the par-collapse opt-option also

But anyway, if you write it as a built-in, I don't see how you can glue it with a compiler pass, unless you copy-paste the equation definition?

@georgebisbas
Copy link
Contributor Author

But anyway, if you write it as a built-in, I don't see how you can glue it with a compiler pass, unless you copy-paste the equation definition?

Yes, though I am still thinking about the best way for doing that.

@georgebisbas georgebisbas changed the title builtins: Add devito version of NumPy's count_nonzero builtins: Add devito version of NumPy's count_nonzero, nonzero Dec 28, 2021
@georgebisbas georgebisbas force-pushed the devito_nonzero branch 3 times, most recently from b7abe2a to e3aac66 Compare December 29, 2021 13:57
@georgebisbas
Copy link
Contributor Author

But anyway, if you write it as a built-in, I don't see how you can glue it with a compiler pass, unless you copy-paste the equation definition?

Yes, since it will be added as part of the pass, we will need to use the equations of it. However, are we opposed to having devito versions of this?

@georgebisbas georgebisbas marked this pull request as ready for review January 5, 2022 09:57
eqi = dv.Eq(i, 0)

eq0 = dv.Inc(g[i], 1, implicit_dims=(f.dimensions + (ci,)))
op0 = dv.Operator([eqi, eq0], opt=('openmp', {'par-collapse-ncores': 30}))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really the most efficient way to compute this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrt par-collpase?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I mean the equation + conditional but I guess not really simpler way.

@mloubout
Copy link
Contributor

Where would this be used? And when you say faster does it count the code-generation overhead because it would re-generate the operator at each call so numpy is still probably more efficient.

@georgebisbas
Copy link
Contributor Author

Where would this be used? And when you say faster does it count the code-generation overhead because it would re-generate the operator at each call so numpy is still probably more efficient.

Sorry to reply late Mathias. With regards to the use, maybe it is not needed anymore for the time-tiling work, since I can use this: https://github.com/devitocodes/devito/pull/1817/files#diff-2da23b1e569f0f2b184634790719bca628a15fdd92739ddf6e9f0452e5b9bc6eR614

wrt to efficiency I admit I did not check the operator generation.

The question is: Since we probably do not need this functionality, I am thinking of whether this can be an enhancement/tutorial of how to use conditional dimension? Thoughts @FabioLuporini ?

@FabioLuporini
Copy link
Contributor

@georgebisbas perhaps you can extend the existing notebook about ConditionalDimensions with one/two/three cells showing this additional example? probably this PR can be closed though?

@georgebisbas
Copy link
Contributor Author

I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API api (symbolics, types, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants