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/472 matmul updates #473

Merged
merged 16 commits into from
Feb 7, 2020
Merged

Bug/472 matmul updates #473

merged 16 commits into from
Feb 7, 2020

Conversation

coquelin77
Copy link
Member

Description

Updates to the matmul and dot functions to increase their

Issue/s resolved: #472

Changes proposed:

  • flag added to matmul to avoid resplit of the first argument (a)
  • dot not returns a split=None vector of both vectors are split=None

Type of change

Remove irrelevant options:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Due Diligence

  • All split configurations tested
  • Multiple dtypes tested in relavent 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?

It should not change the behavior of other functions but if they use dot and expect a split axis this may need to be addressed.

@codecov
Copy link

codecov bot commented Feb 5, 2020

Codecov Report

Merging #473 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #473   +/-   ##
=======================================
  Coverage   96.72%   96.72%           
=======================================
  Files          60       60           
  Lines       12278    12278           
=======================================
  Hits        11876    11876           
  Misses        402      402

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 69c1c57...97e1812. Read the comment docs.

Copy link
Contributor

@Cdebus Cdebus left a comment

Choose a reason for hiding this comment

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

I'm not quite sure if it is sufficient to only consider the vector case for this.
When I implemented the resplit function for out-of-place case, I adapted the HeAT functions, however did not change the current implemenation design (aka most functions kept the inplace resplit) since I didn't know enough on the algorithm design (especially in matmul).
However, now might be a good design to discuss the overall behaviour and API, and wheter matmul should change the splitting of the inputs inplace.

heat/core/linalg.py Outdated Show resolved Hide resolved
heat/core/linalg.py Show resolved Hide resolved
@coquelin77
Copy link
Member Author

I'm not quite sure if it is sufficient to only consider the vector case for this.
When I implemented the resplit function for out-of-place case, I adapted the HeAT functions, however did not change the current implemenation design (aka most functions kept the inplace resplit) since I didn't know enough on the algorithm design (especially in matmul).
However, now might be a good design to discuss the overall behaviour and API, and wheter matmul should change the splitting of the inputs inplace.

it should only resplit in the case that is mentioned. but this could be done out of place as well, although it would require a copy call.

@rainman110
Copy link
Member

rainman110 commented Feb 5, 2020

@coquelin77 I have some notes to your fix

  • It feels still wierd in a functional interface, when input args are changed after the call. IMHO a should be copied to a temporary before resplit. I know, that this could be a performance issue though
  • the new argument no_resplit=False feels a bit strange from the api design as it uses a double negative. Why not turning it around, i.e. allow_resplit=True by default?

@coquelin77
Copy link
Member Author

@rainman110 @Cdebus I changed the default behavior to be the case that the split axis is not changed. I think that this solution is the most clear for users, those with more experience can choose if they want to allow for 'a' to be split or not

@rainman110
Copy link
Member

@coquelin77 Sounds good to me! Thank you very much!

@coquelin77
Copy link
Member Author

codecov is apparently reading a coverage change in kmeans but i believe that this is due to it reading the wrong base commit. there are no changes made to kmeans in this PR according to github

@Cdebus
Copy link
Contributor

Cdebus commented Feb 7, 2020

Nice work!

@Cdebus Cdebus merged commit 86f3f45 into master Feb 7, 2020
@Markus-Goetz Markus-Goetz deleted the bug/472-matmul-updates branch April 7, 2020 11:11
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.

Unsplit matrix multiplication changes split of first matrix argument
3 participants