FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

compiler: misc sparse bug fixes by mloubout · Pull Request #2912 · devitocodes/devito · GitHub

/ devito Public

compiler: misc sparse bug fixes - #2912

Merged
mloubout merged 4 commits into
mainfrom
misc-sparse-fix
May 4, 2026
ConversationCommits (4)ChecksFiles changed
Merged

compiler: misc sparse bug fixes#2912
mloubout merged 4 commits into
mainfrom
misc-sparse-fix

Conversation

mloubout commented May 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Fix bug with complex sparse functions
  • Fix bug with split kernel invalid code with multiple operations

See

https://devitocodes.slack.com/archives/CVC6JFZ4N/p1777774975142759?thread_ts=1777774396.183819&cid=CVC6JFZ4N

mloubout added the bug-py label May 3, 2026
mloubout force-pushed the misc-sparse-fix branch from 80ae111 to 32c40b2 Compare May 3, 2026 18:31

codecov Bot commented May 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.38%. Comparing base (3d5bb25) to head (a54b3c6).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2912   +/-   ##
=======================================
  Coverage   83.37%   83.38%           
=======================================
  Files         248      248           
  Lines       51662    51685   +23     
  Branches     4459     4460    +1     
=======================================
+ Hits        43073    43096   +23     
  Misses       7834     7834           
  Partials      755      755
Flag Coverage Δ
pytest-gpu-aomp-amdgpuX 68.69% <100.00%> (-0.01%) ⬇️
pytest-gpu-gcc- 78.00% <97.14%> (+0.01%) ⬆️
pytest-gpu-icx- 77.61% <97.14%> (+<0.01%) ⬆️
pytest-gpu-nvc-nvidiaX 69.24% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mloubout force-pushed the misc-sparse-fix branch 3 times, most recently from 775e909 to 6367e85 Compare May 4, 2026 03:50

review-notebook-app Bot commented May 4, 2026

Copy link
Copy Markdown

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

FabioLuporini reviewed May 4, 2026
View reviewed changes
Comment thread devito/operations/interpolators.py
return self.grid.dimensions

@cached_property
@property

FabioLuporini May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

bad sign -- is it symptomatic of a change of state?

mloubout May 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

self.sfunction._crdim is memoized, caching on top doesn't have any benefit and would actually double cache it

FabioLuporini May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

that's in a different class and we shouldn't generally rely on that, so I'd keep the cached_property

Comment thread devito/types/sparse.py
return self._dist_origin

@memoized_meth
def _crdim(self, dim):

FabioLuporini May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

unclear what a "crdim" is, so I think a docstring is due

mloubout May 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

"custom radius dimension" but sure can add docstring

Comment thread devito/types/sparse.py
self.r, 2*self.r, self._sparse_dim)

@memoized_meth
def _cond_rdim(self, dim, cond):

FabioLuporini May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I also struggle with this name

mloubout force-pushed the misc-sparse-fix branch from 6367e85 to 470cad6 Compare May 4, 2026 11:07
mloubout force-pushed the misc-sparse-fix branch from 470cad6 to 1520fa2 Compare May 4, 2026 11:09
FabioLuporini approved these changes May 4, 2026
View reviewed changes
Comment thread devito/operations/interpolators.py
return self.grid.dimensions

@cached_property
@property

FabioLuporini May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

that's in a different class and we shouldn't generally rely on that, so I'd keep the cached_property

Comment thread devito/types/sparse.py
def _crdim(self, dim):
"""
The CustomDimension associated with the Dimension `dim` for
the radius of the interpolation/injection stencil

FabioLuporini May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ultra-nitpick: full stop

Comment thread devito/types/sparse.py
@memoized_meth
def _cond_rdim(self, dim, cond):
"""
The interpolation/injection radius dimension with guard bounds

FabioLuporini May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ultra-nitpick: full stop

FabioLuporini May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

maybe also "guarded"?

mloubout merged commit 0e760e8 into main May 4, 2026
42 checks passed
mloubout deleted the misc-sparse-fix branch May 4, 2026 14:53
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

FabioLuporini FabioLuporini approved these changes

Assignees

No one assigned

Labels

bug-py

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL