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

Bugfix: _like factories now default to same device as input DNDarray #1443

Merged
merged 17 commits into from
May 7, 2024

Conversation

mrfh92
Copy link
Collaborator

@mrfh92 mrfh92 commented Apr 18, 2024

Due Diligence

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

Description

Issue/s resolved: #1464

Changes proposed:

Edited by @ClaudiaComito on Apr 30

  • changed default device in factories.__factory_like(a), now device defaults to a.device unless explicitly set
  • addressed pre-commit complaints
  • removed unnecessary/legacy numpy() calls from test_random to facilitate testing on AMD runner

Type of change

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

Memory requirements

NA

Performance

test_random now takes a bit longer as it's running statistical functions on distributed DNDarrays instead of on the gathered numpy arrays

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

no

@mrfh92
Copy link
Collaborator Author

mrfh92 commented Apr 18, 2024

First bug fix for v1.4 (unfortunately we need to release this as it makes problems in one of our benchmarks)

background: this can't be found in our tests as HEAT_TEST_USE_DEVICE=gpu seems to set the default for devices to "gpu", whereas in the standard mode we have default = "cpu" at many places ... therefore, errors of this kind are only found when working with device="gpu" manually in a code

@mrfh92 mrfh92 requested review from ClaudiaComito and mtar April 18, 2024 14:44
Copy link
Contributor

Thank you for the PR!

Copy link

codecov bot commented Apr 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.58%. Comparing base (f465ef0) to head (91f0b4b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1443      +/-   ##
==========================================
- Coverage   91.58%   91.58%   -0.01%     
==========================================
  Files          80       80              
  Lines       11647    11652       +5     
==========================================
+ Hits        10667    10671       +4     
- Misses        980      981       +1     
Flag Coverage Δ
unit 91.58% <100.00%> (-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.

@mtar mtar added bug Something isn't working backport release/1.4.x labels Apr 19, 2024
Copy link
Contributor

Thank you for the PR!

@ClaudiaComito ClaudiaComito added this to the 1.4.1 milestone Apr 22, 2024
Copy link
Contributor

Thank you for the PR!

@ClaudiaComito
Copy link
Contributor

Error in random module in AMD runner, similar but not same as #1289

Copy link
Contributor

Thank you for the PR!

@ClaudiaComito
Copy link
Contributor

@mrfh92 it should work now. I don't know why pre-commit fails.

@ClaudiaComito ClaudiaComito changed the title bugfix: added missing specifications for device Bugfix: _like factories now default to same device as input DNDarray Apr 29, 2024
Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

@mtar mtar marked this pull request as draft May 6, 2024 13:54
@mtar mtar marked this pull request as ready for review May 6, 2024 13:54
Copy link
Contributor

github-actions bot commented May 6, 2024

Thank you for the PR!

2 similar comments
Copy link
Contributor

github-actions bot commented May 6, 2024

Thank you for the PR!

Copy link
Contributor

github-actions bot commented May 6, 2024

Thank you for the PR!

@mtar mtar added merge queue and removed PR talk labels May 7, 2024
@mtar mtar merged commit 421c868 into main May 7, 2024
51 checks passed
Copy link
Contributor

github-actions bot commented May 7, 2024

Successfully created backport PR for release/1.4.x:

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

Successfully merging this pull request may close these issues.

[Bug]: _like factory functions mistakenly set device to Default
3 participants