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

Random: Replaced factories.array with DNDarray #960

Merged
merged 6 commits into from
Apr 21, 2022

Conversation

shahpratham
Copy link
Collaborator

Description

Replaced factories.array with DNDarray in random.py
Issue/s resolved: #800

Changes proposed:

  • factories.array -> dndarray.DNDarray

Type of change

enhancement

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?

no

@mtar
Copy link
Collaborator

mtar commented Apr 9, 2022

GPU cluster tests are currently disabled on this Pull Request.

@ghost
Copy link

ghost commented Apr 9, 2022

CodeSee Review Map:

Review these changes using an interactive CodeSee Map

Review in an interactive map

View more CodeSee Maps

Legend

CodeSee Map Legend

@shahpratham
Copy link
Collaborator Author

@ClaudiaComito @mtar Please tell if any changes are required.

Copy link
Collaborator

@mtar mtar left a comment

Choose a reason for hiding this comment

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

Please, also look at the comments @ClaudiaComito wrote in the similar PRs #949 and #951 for more tips.

return factories.array(data, dtype=x.dtype, is_split=x.split, device=x.device, comm=x.comm)
return DNDarray(
data,
gshape=tuple(data.shape),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Take a closer look at the gshape parameter in the documentation of DNDarray and the tensor you derive the shape from.

Hint

PyTorch tensors are local

Comment on lines 682 to 698
return factories.array(perm, dtype=dtype, device=device, split=split, comm=comm)
return DNDarray(
perm,
gshape=tuple(perm.shape),
dtype=dtype,
device=device,
split=split,
comm=comm,
balanced=True,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Take a look at the split parameter in the documentation of ht.array and compare it to DNDarray.

@codecov
Copy link

codecov bot commented Apr 13, 2022

Codecov Report

Merging #960 (9c936aa) into main (0510437) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #960   +/-   ##
=======================================
  Coverage   95.36%   95.36%           
=======================================
  Files          64       64           
  Lines        9898     9898           
=======================================
  Hits         9439     9439           
  Misses        459      459           
Flag Coverage Δ
gpu 94.59% <100.00%> (ø)
unit 90.98% <100.00%> (ø)

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

Impacted Files Coverage Δ
heat/core/random.py 99.66% <100.00%> (ø)

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 0510437...9c936aa. Read the comment docs.

@shahpratham
Copy link
Collaborator Author

shahpratham commented Apr 13, 2022

@mtar I have made some changes, could you please review.

@mtar
Copy link
Collaborator

mtar commented Apr 14, 2022

Thank you very much @shahpratham, this looks better now.

@ClaudiaComito
Copy link
Contributor

run tests

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.

Thanks a lot @shahpratham ! Can you update the CHANGELOG?

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.

Well done @shahpratham !

@ClaudiaComito
Copy link
Contributor

run tests

@shahpratham shahpratham requested a review from mtar April 20, 2022 15:48
@ClaudiaComito ClaudiaComito merged commit 4490237 into helmholtz-analytics:main Apr 21, 2022
@mtar mtar removed the PR talk label Sep 11, 2023
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.

Module random: replace factories.array() with DNDarray construct where possible
3 participants