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

Support multiple axes for ht.percentile #1510

Merged
merged 42 commits into from
Sep 5, 2024

Conversation

ClaudiaComito
Copy link
Contributor

@ClaudiaComito ClaudiaComito commented Jun 4, 2024

Due Diligence

  • General:
  • Implementation:
    • unit tests: all split configurations tested
    • unit tests: multiple dtypes tested
    • documentation updated where needed

Description

No changes in functionality here, Tuple axes are now supported. The function is just as slow as lamented in #1389 , but it has been refactored for better readability.

Issue/s resolved: None
Related issue: #1420

Changes proposed:

  • refactor and use indexing instead of Bcast
  • support tuple axes in ht.percentile
  • support tuple axes in ht.expand_dims
    ADDED SEPT 4 2024:
  • (following discussion with @JuanPedroGHM ) if reshape of a distributed array involves reducing or expanding dimensions, the output (reshaped array) will be distributed along axis 0 by default. Users can always set the new_axis argument to a different split axis.

Type of change

New feature (non-breaking change which adds functionality) plus refactoring

Memory requirements

Performance

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

yes, reshape, see above

@ClaudiaComito ClaudiaComito added this to the 1.5.0 milestone Jun 4, 2024
@ClaudiaComito ClaudiaComito marked this pull request as draft June 4, 2024 13:14
Copy link
Contributor

github-actions bot commented Jun 4, 2024

Thank you for the PR!

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

Attention: Patch coverage is 99.35484% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.07%. Comparing base (d61782a) to head (e77f1ff).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
heat/core/statistics.py 99.07% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1510   +/-   ##
=======================================
  Coverage   92.06%   92.07%           
=======================================
  Files          83       83           
  Lines       12194    12196    +2     
=======================================
+ Hits        11227    11230    +3     
+ Misses        967      966    -1     
Flag Coverage Δ
unit 92.07% <99.35%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ClaudiaComito ClaudiaComito marked this pull request as ready for review June 5, 2024 14:35
Copy link
Contributor

github-actions bot commented Jun 5, 2024

Thank you for the PR!

@ClaudiaComito ClaudiaComito changed the title Refactor ht.percentile Support multiple axes for ht.percentile Jun 6, 2024
Copy link
Contributor

github-actions bot commented Jun 7, 2024

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

github-actions bot commented Sep 3, 2024

Thank you for the PR!

1 similar comment
Copy link
Contributor

github-actions bot commented Sep 3, 2024

Thank you for the PR!

@ClaudiaComito ClaudiaComito marked this pull request as ready for review September 4, 2024 10:06
Copy link
Contributor

github-actions bot commented Sep 4, 2024

Thank you for the PR!

@ClaudiaComito
Copy link
Contributor Author

@mrfh92 @JuanPedroGHM I think I'm done here.

@mrfh92 I expanded the tests to cover tuple axes for sketched percentile, and also writing to out buffer. @JuanPedroGHM and I also decided to make some changes in reshape, see expanded description above. Let me know if something's missing, thanks!

mrfh92
mrfh92 previously approved these changes Sep 4, 2024
Copy link
Collaborator

@mrfh92 mrfh92 left a comment

Choose a reason for hiding this comment

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

My change requests have been addressed.
Thanks for the work 👍

Copy link
Contributor

github-actions bot commented Sep 5, 2024

Thank you for the PR!

@ClaudiaComito
Copy link
Contributor Author

@mrfh92 I made small edits to the docs to trigger the pipeline again, codecov had gotten stuck. I'm afraid you need to approve again, thanks!

@ClaudiaComito ClaudiaComito merged commit bac864d into main Sep 5, 2024
43 checks passed
@mrfh92 mrfh92 deleted the features/1389-Speed_up_ht_percentile branch September 5, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants