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

examples: Add nonzero example to ConditionalDimension tutorial #1820

Merged
merged 3 commits into from
Feb 8, 2022

Conversation

georgebisbas
Copy link
Contributor

@georgebisbas georgebisbas commented Jan 17, 2022

Add a "nonzero" example to ConditionalDimension tutorial.

Example to :

  • Count nonzeros
  • Return nonzero positions

Continuing from the closed PR: #1815

@georgebisbas georgebisbas added API api (symbolics, types, ...) examples examples labels Jan 17, 2022
@georgebisbas georgebisbas self-assigned this Jan 17, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Jan 17, 2022

Codecov Report

Merging #1820 (22ac819) into master (543b7be) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1820   +/-   ##
=======================================
  Coverage   89.54%   89.54%           
=======================================
  Files         209      209           
  Lines       34517    34517           
  Branches     5212     5212           
=======================================
  Hits        30908    30908           
  Misses       3117     3117           
  Partials      492      492           

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 543b7be...22ac819. Read the comment docs.

@@ -71,7 +71,7 @@
"name": "stderr",
Copy link
Contributor

Choose a reason for hiding this comment

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

Line #10.    i = Scalar(name='i', dtype=np.int32)

That looks weird, what's the actual issue? Can you open an issue for it?


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code, if optimized with openmp+ reduction fails with gcc-5:

  START_TIMER(section0)
  for (int x = x_m; x <= x_M; x += 1)
  {
    for (int y = y_m; y <= y_M; y += 1)
    {
      if (f[x][y] != 0)
      {
        int r0 = 1;
        g[1] += r0;
      }
    }
  }
  STOP_TIMER(section0,timers)

The trick in the PR helps to generate

  int i = 0;

  /* Begin section0 */
  START_TIMER(section0)
  for (int x = x_m; x <= x_M; x += 1)
  {
    for (int y = y_m; y <= y_M; y += 1)
    {
      if (f[x][y] != 0)
      {
        int r0 = 1;
        g[i + 1] += r0;
      }
    }
  }
  STOP_TIMER(section0,timers)
  /* End section0 */

which is valid. Since we are now in a notebook which is not tested for gcc-5, I can get rid of the trick.
Remember that before it was not in notebook, so this was a problem. DO you think it still deserves an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's odd code looks sane enough for GCC what the compilation error?

@georgebisbas georgebisbas requested a review from EdCaunt January 25, 2022 10:22
@georgebisbas georgebisbas changed the title [WIP, RFC] examples: Add nonzero example to ConditionalDimension tutorial examples: Add nonzero example to ConditionalDimension tutorial Jan 25, 2022
Copy link
Contributor

@EdCaunt EdCaunt left a comment

Choose a reason for hiding this comment

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

Looks good to me

"cell_type": "markdown",
"metadata": {},
"source": [
"# Use case C (Count nonzero elements/ Find nonzero positions)\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

More than a "use case", this is an example.

Also, what the example is about should not end up within parentheses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to be consistent with the rest of the notebook. SOunds good to apply this change to previous use-cases/examples?

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.

minor tweaks necessary, but then nearly GTG

"source": [
"# Example A - Count nonzero elements\n",
"\n",
"Here we present an of using `ConditionalDimension`. In the following example, an `Operator` counts the nonzero elements of an array."
Copy link
Contributor

Choose a reason for hiding this comment

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

"an of using" -- to be fixed @georgebisbas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

"\n",
"Here we present another example of using `ConditionalDimension`. In the following example, an `Operator` counts the nonzero elements of an array.\n",
"\n",
"Now, let's take it a step further to return the nonzero positions in $f$. We know that we have `g.data[0]` nonzero elements. In this example, we use the Devito DSL to generate code that will save the nonzero positions."
Copy link
Contributor

Choose a reason for hiding this comment

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

shuold be `f` not $f$ , right?

Copy link
Contributor

Choose a reason for hiding this comment

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

In this example, we use the Devito DSL to generate code that will save the nonzero positions."

background noise, drop

you already have said what you do in the previous paragraph, ie

"In the following example, an Operator counts the nonzero elements of an array."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rewritten, hope its better now.

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.

I also think this notebook has grown substantially verbose, as we're printing the whole Operator code, for every Operator in the notebook. Maybe we should only print the relevant section, ie the time loop ...

@georgebisbas
Copy link
Contributor Author

I also think this notebook has grown substantially verbose, as we're printing the whole Operator code, for every Operator in the notebook. Maybe we should only print the relevant section, ie the time loop ...

Done. Only the "IMPORTANT" part is now printed. (no time loop in the examples though...!)

@georgebisbas georgebisbas removed the API api (symbolics, types, ...) label Feb 7, 2022
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.

Now it LGTM. Thanks, merging

@FabioLuporini FabioLuporini merged commit e2321f4 into master Feb 8, 2022
@FabioLuporini FabioLuporini deleted the nonzero_tutorial branch February 8, 2022 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants