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

DNDarray getitem: corrected split logic when ints in key #732

Merged
merged 5 commits into from
Mar 17, 2021

Conversation

coquelin77
Copy link
Member

Description

Addressed getitem split bug

Issue/s resolved: #730

Changes proposed:

  • change in logic for the new split calculation in getitem when ints are in the key

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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?

It might need change things which relied upon the buggy code. however the tests run clean, so hopefully this just wasnt noticed previously.

@codecov
Copy link

codecov bot commented Mar 9, 2021

Codecov Report

Merging #732 (50063cf) into master (8070fad) will increase coverage by 0.00%.
The diff coverage is 95.65%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #732   +/-   ##
=======================================
  Coverage   95.82%   95.82%           
=======================================
  Files          63       63           
  Lines        8069     8071    +2     
=======================================
+ Hits         7732     7734    +2     
  Misses        337      337           
Flag Coverage Δ
gpu 94.93% <95.65%> (+<0.01%) ⬆️
unit 94.20% <95.65%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
heat/core/dndarray.py 96.48% <95.65%> (+<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 8070fad...50063cf. Read the comment docs.

mtar
mtar previously approved these changes Mar 10, 2021
Copy link
Contributor

@ClaudiaComito ClaudiaComito left a comment

Choose a reason for hiding this comment

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

Great, thanks for this.
I've fixed the problem for reductions operations in #744.

@ClaudiaComito ClaudiaComito self-requested a review March 16, 2021 17:58
Copy link
Contributor

@ClaudiaComito ClaudiaComito left a comment

Choose a reason for hiding this comment

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

@coquelin77 I just realized both #730 fixes (#732 and former #744) must branch off 0.5.x, not off the main branch. We can then release the bug fixes right away without waiting for 0.6.0.

@ClaudiaComito
Copy link
Contributor

@coquelin77 I just realized both #730 fixes (#732 and former #744) must branch off 0.5.x, not off the main branch. We can then release the bug fixes right away without waiting for 0.6.0.

Alright, I think I'm taking this back. The #744 fix already uses some of the 0.6 features and I'm not sure walking the code back to 0.5 state is worth the time. I'll reopen #744 as is and reapprove this one as is as well.

ClaudiaComito
ClaudiaComito previously approved these changes Mar 16, 2021
@coquelin77
Copy link
Member Author

the master merge cleared the approval from @ClaudiaComito but i dont think that we need to go through it again. i will merge it once the tests run throguh

@coquelin77 coquelin77 merged commit 442c271 into master Mar 17, 2021
@coquelin77 coquelin77 deleted the bug/730-getitem-split-axis branch March 17, 2021 10:28
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.

Wrong split axis after summing along axis or indexing
3 participants