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

Features/1448 Refactor random module #1508

Merged
merged 18 commits into from
Jul 22, 2024

Conversation

mrfh92
Copy link
Collaborator

@mrfh92 mrfh92 commented Jun 3, 2024

Due Diligence

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

Description

Issue/s resolved: #1448

Changes proposed:

  • default RNG is now "Batchparallel", i.e. choose a global seed, get process-local seeds by "global seed + rank" on each MPI-rank, use torch's RNG with these local seeds
  • existing Threefry RNG can be used as user-specified option using set_state(...).

Type of change

should not break existing applications as far as they do not rely on strong reproducibility ensured by Threefry but not batchparallel RNG

Performance

likely to improve performance of random number generation compared to existing solution

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

yes, everything related to or using randomness in Heat

@mrfh92
Copy link
Collaborator Author

mrfh92 commented Jun 3, 2024

@ClaudiaComito @JuanPedroGHM @mtar @Markus-Goetz What do you think about the alternative approach to generate random arrays shown here for randn and rand? Does it do the job or are there any objections?

@mrfh92 mrfh92 added PR talk ESAPCA relevant for the ESA-funded project "ESAPCA" labels Jun 3, 2024
@mrfh92 mrfh92 self-assigned this Jun 3, 2024
@mrfh92 mrfh92 added random enhancement New feature or request labels Jun 3, 2024
@mrfh92
Copy link
Collaborator Author

mrfh92 commented Jun 3, 2024

(The error in the other random functions is due to the fact that the way of setting the seed in my new implementation interferes with setting the seed in the other functionality. This would have to be removed if we want to keep the new idea.)

@mrfh92 mrfh92 removed the PR talk label Jun 10, 2024
@mrfh92
Copy link
Collaborator Author

mrfh92 commented Jun 28, 2024

still to be done: figure out influence of proposed changes on other functions

Copy link
Contributor

Thank you for the PR!

1 similar comment
Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

github-actions bot commented Jul 2, 2024

Thank you for the PR!

1 similar comment
Copy link
Contributor

github-actions bot commented Jul 2, 2024

Thank you for the PR!

…tead of default 1e-8) as otherwise the test fails
Copy link
Contributor

github-actions bot commented Jul 2, 2024

Thank you for the PR!

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.04%. Comparing base (fea923b) to head (803a016).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1508      +/-   ##
==========================================
+ Coverage   92.00%   92.04%   +0.04%     
==========================================
  Files          83       83              
  Lines       12072    12109      +37     
==========================================
+ Hits        11107    11146      +39     
+ Misses        965      963       -2     
Flag Coverage Δ
unit 92.04% <100.00%> (+0.04%) ⬆️

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.

@mrfh92 mrfh92 requested a review from mtar July 3, 2024 13:17
Copy link
Contributor

github-actions bot commented Jul 5, 2024

Thank you for the PR!

@mrfh92 mrfh92 changed the title Features/1448 random arrays of arbitrary size Features/1448 Refactor random module Jul 8, 2024
@ClaudiaComito ClaudiaComito added this to the 1.5.0 milestone Jul 8, 2024
heat/core/random.py Outdated Show resolved Hide resolved
heat/core/random.py Outdated Show resolved Hide resolved
heat/core/random.py Outdated Show resolved Hide resolved
heat/core/random.py Outdated Show resolved Hide resolved
Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

@mrfh92 mrfh92 requested a review from mtar July 15, 2024 10:27
@mtar
Copy link
Collaborator

mtar commented Jul 15, 2024

few lines missing coverage

Copy link
Contributor

Thank you for the PR!

@mrfh92
Copy link
Collaborator Author

mrfh92 commented Jul 15, 2024

something strange happened with the codecov

Copy link
Contributor

Thank you for the PR!

@mrfh92
Copy link
Collaborator Author

mrfh92 commented Jul 18, 2024

@mtar now we have 100% coverage of patch

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.

Looks fine to me 👌

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.

Checks fail on older PyTorch versions < 2.0 with IndexError. Needs to be fixed or we merge it after the minimum version has been raised. See #1572

@mrfh92
Copy link
Collaborator Author

mrfh92 commented Jul 19, 2024

@mtar I have created #1573 to solve this problem.

@mtar
Copy link
Collaborator

mtar commented Jul 19, 2024

CI Bug after #1573 addressed in #1576

Copy link
Contributor

Thank you for the PR!

@mrfh92
Copy link
Collaborator Author

mrfh92 commented Jul 19, 2024

@mtar now it workes?

@mrfh92 mrfh92 merged commit b153d30 into main Jul 22, 2024
35 checks passed
@mrfh92 mrfh92 deleted the features/1448-random_arrays_of_arbitrary_size branch July 22, 2024 07:23
@mrfh92
Copy link
Collaborator Author

mrfh92 commented Jul 22, 2024

@mtar thanks for reviewing 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ESAPCA relevant for the ESA-funded project "ESAPCA" random
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow random arrays of arbitrary size / speed up random arrays
3 participants