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

Outhalo size warning #1493

Merged
merged 3 commits into from
Nov 9, 2020
Merged

Outhalo size warning #1493

merged 3 commits into from
Nov 9, 2020

Conversation

rhodrin
Copy link
Contributor

@rhodrin rhodrin commented Nov 5, 2020

Tiny PR to fix #1466 .

@rhodrin rhodrin changed the title dense: Fix outhalo size warning to check correct dimensions. Outhalo size warning Nov 5, 2020
@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #1493 (9844edf) into master (b5439c8) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1493      +/-   ##
==========================================
- Coverage   86.52%   86.50%   -0.02%     
==========================================
  Files         197      197              
  Lines       28504    28506       +2     
  Branches     3873     3875       +2     
==========================================
- Hits        24662    24660       -2     
- Misses       3408     3411       +3     
- Partials      434      435       +1     
Impacted Files Coverage Δ
devito/types/dense.py 93.80% <100.00%> (+0.02%) ⬆️
devito/compiler.py 53.28% <0.00%> (-1.32%) ⬇️

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 b5439c8...9844edf. Read the comment docs.

@mloubout mloubout added the MPI mpi-related label Nov 6, 2020
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 fine, any way to test it?

@FabioLuporini
Copy link
Contributor

I'm not sure I understand the fix. self._decomposition should have been already of the proper size, and so left and right, given that left = [.... in zip(self._decomposition, ...)]. Why do you need to further slice it?

@rhodrin
Copy link
Contributor Author

rhodrin commented Nov 6, 2020

I'm not sure I understand the fix. self._decomposition should have been already of the proper size, and so left and right, given that left = [.... in zip(self._decomposition, ...)]. Why do you need to further slice it?

For TimeFunction's left and right also contain the size of the time halo which we wan't to ignore for this spatial check.

@rhodrin
Copy link
Contributor Author

rhodrin commented Nov 6, 2020

Looks fine, any way to test it?

I was thinking about that, but came to the conclusion that a specific test was overkill. The incorrect warning currently (<- current master) shows up for the mpi seismic examples, but I'm not sure we wan't to play around with them or add a 'dummy' example that currently erronously produces the warning?

@FabioLuporini FabioLuporini merged commit dc504d6 into master Nov 9, 2020
@FabioLuporini
Copy link
Contributor

Thanks, merged.

@FabioLuporini FabioLuporini deleted the fix_halo_warning branch November 9, 2020 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MPI mpi-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Erronous halo size warning
3 participants