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

Features/179 repeat #674

Merged
merged 53 commits into from
Oct 26, 2020
Merged

Features/179 repeat #674

merged 53 commits into from
Oct 26, 2020

Conversation

lenablind
Copy link
Collaborator

@lenablind lenablind commented Sep 18, 2020

Description

Implementation of function repeat() based on np.repeat()
For process-local operations, I use torch.repeat_interleave

Docs numpy: https://numpy.org/doc/stable/reference/generated/numpy.repeat.html
Docs pytorch: https://pytorch.org/docs/stable/generated/torch.repeat_interleave.html

Strategy

In the following, only the distributed case (<=> a.split != None) will be explained more specifically as the algorithm otherwise results mainly in a direct call of the torch function.

axis is None

Implies a.split = 0 as the (total) result would be in the wrong order otherwise.
To assure the correct distribution of repeats and syntactical consistency with numpy (repeats must be 1-dimensional), repeats has to be reshaped to the global shape of a, redistributed along axis 0 and be flattened afterwards.
The last step is necessary due to compatibility with torch (1-dimensionality required as in numpy).

axis is not None

Depending on whether a.split == axis, repeats has to be either split along axis 0 (case True) or gathered on all processes (case False). The eventually resulting redistribution of data is needed to assure correct local and therefore global results.

Issue/s resolved: #179

Changes proposed:

  • Implemented new function: manipulations.repeat()
  • Implemented corresponding tests

Type of change

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

Due Diligence

  • All split configurations tested
  • Multiple dtypes tested in relevant functions
  • Documentation updated (if needed)
  • Updated changelog.md under the title "Pending Additions"

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

no

@mtar
Copy link
Collaborator

mtar commented Sep 18, 2020

GPU cluster tests are currently disabled on this Pull Request.

@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #674 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #674      +/-   ##
==========================================
+ Coverage   97.47%   97.53%   +0.05%     
==========================================
  Files          87       87              
  Lines       17231    17643     +412     
==========================================
+ Hits        16796    17208     +412     
  Misses        435      435              
Impacted Files Coverage Δ
heat/core/manipulations.py 99.28% <100.00%> (+0.06%) ⬆️
heat/core/tests/test_factories.py 100.00% <100.00%> (ø)
heat/core/tests/test_manipulations.py 99.94% <100.00%> (+0.01%) ⬆️

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 5cd2b42...229585a. Read the comment docs.

Copy link
Member

@coquelin77 coquelin77 left a comment

Choose a reason for hiding this comment

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

looks pretty good. good job!

heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Outdated Show resolved Hide resolved
@lenablind
Copy link
Collaborator Author

looks pretty good. good job!

Thank you!

@mtar
Copy link
Collaborator

mtar commented Sep 23, 2020

ok to test

Copy link
Member

@coquelin77 coquelin77 left a comment

Choose a reason for hiding this comment

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

very very minor changes and only a couple questions. good work!

heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Show resolved Hide resolved
@lenablind
Copy link
Collaborator Author

very very minor changes and only a couple questions. good work!

@coquelin77 Thank you!

@coquelin77 coquelin77 merged commit dc6caa3 into master Oct 26, 2020
@coquelin77 coquelin77 deleted the features/179-repeat branch October 26, 2020 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement repeat()
3 participants