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/178 tile #673

Merged
merged 41 commits into from
Aug 20, 2021
Merged

Features/178 tile #673

merged 41 commits into from
Aug 20, 2021

Conversation

ClaudiaComito
Copy link
Contributor

@ClaudiaComito ClaudiaComito commented Sep 17, 2020

Description

Implementation of heat version of np.tile():
https://numpy.org/doc/stable/reference/generated/numpy.tile.html

  • process-local operations via torch.repeat()
  • distributed tiling via Alltoallv
    - also, introducing sanitation module Implement sanitation module #468 (with only a few basic functions), the idea is to expand it on the fly as needed

Issue/s resolved: #178

Changes proposed:

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

@codecov
Copy link

codecov bot commented Sep 17, 2020

Codecov Report

Merging #673 (d4f450e) into master (e0af0e5) will increase coverage by 0.00%.
The diff coverage is 94.02%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #673    +/-   ##
========================================
  Coverage   95.40%   95.40%            
========================================
  Files          64       64            
  Lines        9246     9380   +134     
========================================
+ Hits         8821     8949   +128     
- Misses        425      431     +6     
Flag Coverage Δ
gpu 94.57% <94.02%> (+0.01%) ⬆️
unit 91.17% <94.02%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
heat/core/manipulations.py 98.56% <94.02%> (-0.52%) ⬇️
heat/core/indexing.py 100.00% <0.00%> (+4.76%) ⬆️

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 e0af0e5...d4f450e. Read the comment docs.

@ClaudiaComito ClaudiaComito added this to In progress in ASSET Support via automation Sep 17, 2020
@ClaudiaComito ClaudiaComito added this to In progress in v0.5.0 via automation Sep 17, 2020
@ClaudiaComito ClaudiaComito added the enhancement New feature or request label Sep 17, 2020
@ClaudiaComito ClaudiaComito marked this pull request as ready for review July 9, 2021 11:58
@ClaudiaComito
Copy link
Contributor Author

ClaudiaComito commented Jul 9, 2021

Is the change of the split axis intended?

>>> ht.tile(ht.array([1,2,3],split=0), [1,1])
DNDarray([[1, 2, 3]], dtype=ht.int64, device=cpu:0, split=1)

@mtar belated thanks for commenting, yes you are tiling a 1-D array with 2-dimensional repetitions, in this case tile() will prepend the missing dimension (just like numpy.tile()), so the split axis nominally increases by one in this case.

@ClaudiaComito ClaudiaComito added this to the 1.2.x milestone Jul 9, 2021
@coquelin77
Copy link
Member

rerun tests

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.

the code itself looks fine, but i found a bug in the alltoallv logic:

x = ht.arange(3*3).reshape((3, 3)).resplit(1)
print(x)
>>> DNDarray([[0, 1, 2],
              [3, 4, 5],
              [6, 7, 8]], dtype=ht.int32, device=cpu:0, split=1)
y = ht.tile(x, (2, 3))
print(y)
>>> DNDarray([[        0,         4,         2,         0,         4,         2,         0,         4,         2],
              [        1,         1,         0,         1,     22065,     32628,         1,     32628,         0],
              [        3,         0,         0,         3,         0, 724118576,         3,         0,         0],
              [        0,         4,         2,         0,         4,         2,         0,         4,         2],
              [        1,         1,         0,         1,     22065,     32628,         1,     32628,         0],
              [        3,         0,         0,         3,         0, 724118576,         3,         0,         0]], dtype=ht.int32, device=cpu:0, split=1)

@ClaudiaComito
Copy link
Contributor Author

the code itself looks fine, but i found a bug in the alltoallv logic:

x = ht.arange(3*3).reshape((3, 3)).resplit(1)
print(x)
>>> DNDarray([[0, 1, 2],
              [3, 4, 5],
              [6, 7, 8]], dtype=ht.int32, device=cpu:0, split=1)
y = ht.tile(x, (2, 3))
print(y)
>>> DNDarray([[        0,         4,         2,         0,         4,         2,         0,         4,         2],
              [        1,         1,         0,         1,     22065,     32628,         1,     32628,         0],
              [        3,         0,         0,         3,         0, 724118576,         3,         0,         0],
              [        0,         4,         2,         0,         4,         2,         0,         4,         2],
              [        1,         1,         0,         1,     22065,     32628,         1,     32628,         0],
              [        3,         0,         0,         3,         0, 724118576,         3,         0,         0]], dtype=ht.int32, device=cpu:0, split=1)

Good catch! Fixed

@coquelin77
Copy link
Member

the code itself looks fine, but i found a bug in the alltoallv logic:

x = ht.arange(3*3).reshape((3, 3)).resplit(1)
print(x)
>>> DNDarray([[0, 1, 2],
              [3, 4, 5],
              [6, 7, 8]], dtype=ht.int32, device=cpu:0, split=1)
y = ht.tile(x, (2, 3))
print(y)
>>> DNDarray([[        0,         4,         2,         0,         4,         2,         0,         4,         2],
              [        1,         1,         0,         1,     22065,     32628,         1,     32628,         0],
              [        3,         0,         0,         3,         0, 724118576,         3,         0,         0],
              [        0,         4,         2,         0,         4,         2,         0,         4,         2],
              [        1,         1,         0,         1,     22065,     32628,         1,     32628,         0],
              [        3,         0,         0,         3,         0, 724118576,         3,         0,         0]], dtype=ht.int32, device=cpu:0, split=1)

Good catch! Fixed

then this is ready as far as i can tell

@coquelin77 coquelin77 merged commit e2f75c3 into master Aug 20, 2021
ASSET Support automation moved this from In progress to Done Aug 20, 2021
@coquelin77 coquelin77 deleted the features/178-tile branch August 20, 2021 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
ASSET Support
  
Done
v0.5.0
  
In progress
Development

Successfully merging this pull request may close these issues.

Implement tile()
3 participants