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

api: Use subs for origin in case index is a function #2120

Merged
merged 3 commits into from
May 15, 2023
Merged

Conversation

mloubout
Copy link
Contributor

@mloubout mloubout commented May 8, 2023

Fixes #1950

@mloubout mloubout added the API api (symbolics, types, ...) label May 8, 2023
@mloubout mloubout requested review from FabioLuporini and EdCaunt May 8, 2023 19:48
@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 May 8, 2023

Codecov Report

Merging #2120 (ad29693) into master (c80b86c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2120   +/-   ##
=======================================
  Coverage   87.17%   87.17%           
=======================================
  Files         222      222           
  Lines       39368    39378   +10     
  Branches     5117     5118    +1     
=======================================
+ Hits        34319    34328    +9     
- Misses       4478     4480    +2     
+ Partials      571      570    -1     
Impacted Files Coverage Δ
devito/operations/interpolators.py 98.79% <100.00%> (+<0.01%) ⬆️
devito/tools/data_structures.py 72.66% <100.00%> (+0.17%) ⬆️
devito/types/basic.py 93.77% <100.00%> (ø)
devito/types/dense.py 93.53% <100.00%> (ø)
tests/test_symbolics.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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.

types isn't a valid tag anymore; you probably should use compiler or dsl

devito/types/basic.py Show resolved Hide resolved
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.

Can we add a small MFE somewhere?

@mloubout mloubout force-pushed the mirror-fd branch 2 times, most recently from 72b0a3a to eb66c63 Compare May 9, 2023 15:20
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.

Would it be worth having a test to double-check that indices are not affected by staggering? Possibly create two simple 1D kernels containing the absolute of an index, one where the function is staggered and one where it is not and check the generated C string is the same.

devito/operations/interpolators.py Outdated Show resolved Hide resolved
@mloubout
Copy link
Contributor Author

Added test


# Apply mapper to each variable with origin correction before the
# dimensions get replaced.
vmapper = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

equivalent to:

subs = {v: v.subs({k: c - v.origin.get(k, 0) for k, c in mapper.items()})}

?

Copy link
Contributor

Choose a reason for hiding this comment

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

why does this issue emerge only now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before we were just doing index - origin during indexification which was making the assumption "indices are of the form i + o" so it didn't matter, but now without the assumption it needs the dimension which is removed and replaced by the ii_rec_... indices so we need to take care of it here when the dimension is removed

devito/operations/interpolators.py Show resolved Hide resolved
@FabioLuporini FabioLuporini changed the title types: use subs for origin in case index is a function compiler: Use subs for origin in case index is a function May 11, 2023
@FabioLuporini FabioLuporini changed the title compiler: Use subs for origin in case index is a function api: Use subs for origin in case index is a function May 11, 2023
@mloubout mloubout force-pushed the mirror-fd branch 4 times, most recently from 5d9a2a2 to 9d4d121 Compare May 12, 2023 12:21
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.

OK, thanks

@mloubout mloubout merged commit 526dcb8 into master May 15, 2023
@mloubout mloubout deleted the mirror-fd branch May 15, 2023 21:30
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.

Staggering not purged when index contains abs() of a value
3 participants