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

Bug/squeeze split semantics #562

Merged
merged 10 commits into from
May 7, 2020
Merged

Bug/squeeze split semantics #562

merged 10 commits into from
May 7, 2020

Conversation

ClaudiaComito
Copy link
Contributor

@ClaudiaComito ClaudiaComito commented Apr 27, 2020

Description

Changes to manipulations.squeeze() to follow split semantics. In the previous version, the output tensor was being "gathered" and the function returned a non-distributed tensor.

Split semantics for squeeze() are now as follows: a distributed tensor will keep its original split dimension after "squeezing", which, depending on the squeeze axis, may result in a lower numerical 'split' value, as in:

    >>> x.shape
    (10, 1, 12, 13)
    >>> x.split
    2
    >>> x.squeeze().shape
    (10, 12, 13)
    >>> x.squeeze().split
    1

The edge case x.split = axis returns a non-distributed tensor.

Issue/s resolved: #561, #273

Changes proposed:

  • I removed the Allgather part and made sure the global output shape was set correctly in returning the DNDarray. I also reinstated the test_squeeze() portion on unevenly distributed tensors.

Type of change

Bug fix, breaking change.

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?

yes if they rely on ht.squeeze() returning a non-distributed tensor.

@ClaudiaComito ClaudiaComito added the bug Something isn't working label Apr 27, 2020
@codecov
Copy link

codecov bot commented Apr 27, 2020

Codecov Report

Merging #562 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #562      +/-   ##
==========================================
+ Coverage   96.44%   96.45%   +0.01%     
==========================================
  Files          75       75              
  Lines       15130    15145      +15     
==========================================
+ Hits        14592    14608      +16     
+ Misses        538      537       -1     
Impacted Files Coverage Δ
heat/core/manipulations.py 99.51% <100.00%> (+0.15%) ⬆️
heat/core/tests/test_manipulations.py 99.39% <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 a1c3792...6ca24b8. Read the comment docs.

@ClaudiaComito ClaudiaComito force-pushed the bug/squeeze-split-semantics branch from 63bab7e to ec2094c Compare April 28, 2020 05:09
@ClaudiaComito ClaudiaComito force-pushed the bug/squeeze-split-semantics branch from ec2094c to 81026e8 Compare April 28, 2020 08:32
@ClaudiaComito ClaudiaComito force-pushed the bug/squeeze-split-semantics branch from 38287e8 to 99904c6 Compare April 29, 2020 07:11
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.

approval pending the addition of 1 more test (or a small change of one). can you make one of them axis=None? just for completeness

@coquelin77
Copy link
Member

sorry, these tests are already included. merging now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ht.squeeze() doesn't follow split semantics
2 participants