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

compiler: Uncache data carriers #1913

Merged
merged 4 commits into from
May 23, 2022
Merged

compiler: Uncache data carriers #1913

merged 4 commits into from
May 23, 2022

Conversation

FabioLuporini
Copy link
Contributor

@FabioLuporini FabioLuporini commented Apr 29, 2022

Scalar data carriers are unique, so why attempt to cache them?

@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #1913 (5b9adbe) into master (265b653) will increase coverage by 0.00%.
The diff coverage is 92.59%.

@@           Coverage Diff           @@
##           master    #1913   +/-   ##
=======================================
  Coverage   89.64%   89.65%           
=======================================
  Files         211      211           
  Lines       35634    35639    +5     
  Branches     5369     5369           
=======================================
+ Hits        31945    31951    +6     
  Misses       3189     3189           
+ Partials      500      499    -1     
Impacted Files Coverage Δ
devito/types/basic.py 95.73% <75.00%> (+0.06%) ⬆️
devito/types/caching.py 92.75% <100.00%> (+0.32%) ⬆️
tests/test_caching.py 99.77% <100.00%> (+<0.01%) ⬆️
examples/seismic/viscoacoustic/operators.py 100.00% <0.00%> (ø)

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 265b653...5b9adbe. Read the comment docs.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@FabioLuporini FabioLuporini force-pushed the uncache-data-carriers branch from 7b61c6b to 5b9adbe Compare May 2, 2022 07:38
@@ -303,7 +303,7 @@
" for (int y = y_m; y <= y_M; y += 1)\n",
" {\n",
" float r3 = -2.0F*u[time][x + 2][y + 2];\n",
" u[time + 1][x + 2][y + 2] = dt*(r0*u[time][x + 2][y + 2] + c*(r1*r3 + r1*u[time][x + 1][y + 2] + r1*u[time][x + 3][y + 2] + r2*r3 + r2*u[time][x + 2][y + 1] + r2*u[time][x + 2][y + 3]));\n",
" u[time + 1][x + 2][y + 2] = dt*(c*(r1*r3 + r1*u[time][x + 1][y + 2] + r1*u[time][x + 3][y + 2] + r2*r3 + r2*u[time][x + 2][y + 1] + r2*u[time][x + 2][y + 3]) + r0*u[time][x + 2][y + 2]);\n",
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 change of order deterministic?

Copy link
Contributor

@mloubout mloubout left a comment

Choose a reason for hiding this comment

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

Looks gtg. Not sue why the symbol order changes from ascending to descending, I guess id 's related.

# Obviously:
assert foo0 is not foo1
assert foo0 is not foo2
assert foo1 is not foo2
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the dtype and value be checked too for safety?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"checked" how? like foo0.dtype is foo1.dtype ? that's obvious imho

@FabioLuporini
Copy link
Contributor Author

@mloubout

Looks gtg. Not sue why the symbol order changes from ascending to descending, I guess id 's related.

Oh, it boils down to how SymPy internally orders objects... it uses the cls.__name__ , and this PR is altering the class hierarchy, so...

@FabioLuporini FabioLuporini merged commit 3bf0a19 into master May 23, 2022
@FabioLuporini FabioLuporini deleted the uncache-data-carriers branch May 23, 2022 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants