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

Support one-sided halo for DNDarrays #1509

Conversation

ClaudiaComito
Copy link
Contributor

@ClaudiaComito ClaudiaComito commented Jun 4, 2024

Due Diligence

  • General:
  • Implementation:
    • unit tests: all split configurations tested
    • unit tests: multiple dtypes tested
    • documentation updated where needed

Description

Enabling one-sided halo for #1419.

Usage (run on n>1 MPI processes):

# one-sided halo: from next rank only
a = ht.arange(100, split=0).reshape(10, 10)
a.get_halo(2, prev=False)
print("Test one-sided halo = ", a.array_with_halos)

Issue/s resolved: #

Changes proposed:

  • introduced boolean kwargs prev and next for DNDarray.get_halo(), both True by default
  • data communication to next MPI rank only if prev == True
  • data communication to the previous MPI rank only if next == True

Type of change

  • New feature (non-breaking change which adds functionality)

Memory requirements

Performance

Does this change modify the behaviour of other functions? If so, which?

no

@ClaudiaComito ClaudiaComito added enhancement New feature or request halo Operations requiering a tensor halo dndarray labels Jun 4, 2024
@ClaudiaComito ClaudiaComito added this to the 1.5.0 milestone Jun 4, 2024
@ClaudiaComito
Copy link
Contributor Author

@FOsterfeld this PR is branched off your unfold branch, feel free to experiment here as needed. If you're ok with it, we can merge this into #1419

Copy link
Contributor

github-actions bot commented Jun 4, 2024

Thank you for the PR!

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.73%. Comparing base (5cdd986) to head (63aa686).

Additional details and impacted files
@@                                             Coverage Diff                                             @@
##           features/1400-Implement_unfold-operation_similar_to_torch_Tensor_unfold    #1509      +/-   ##
===========================================================================================================
- Coverage                                                                    91.73%   91.73%   -0.01%     
===========================================================================================================
  Files                                                                           80       80              
  Lines                                                                        11692    11696       +4     
===========================================================================================================
+ Hits                                                                         10726    10729       +3     
- Misses                                                                         966      967       +1     
Flag Coverage Δ
unit 91.73% <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.

@FOsterfeld FOsterfeld merged commit 9b059f8 into features/1400-Implement_unfold-operation_similar_to_torch_Tensor_unfold Jun 5, 2024
6 checks passed
Copy link
Member

@FOsterfeld FOsterfeld left a comment

Choose a reason for hiding this comment

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

@ClaudiaComito Thanks for this functionality, it made the implementation of #1419 much simpler! I have not tested the method specifically but all unit tests for unfold passed, so it should behave as intended when using prev=False.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dndarray enhancement New feature or request halo Operations requiering a tensor halo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants