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

enhancement: reduced usage of numpy and substituted built-in libraries #8418

Merged
merged 11 commits into from
Oct 18, 2024

Conversation

ajit97singh
Copy link
Contributor

Related Issues

Proposed Changes:

Reduced usage of numpy by substituting math and random libraries

@ajit97singh ajit97singh requested review from a team as code owners September 30, 2024 05:03
@ajit97singh ajit97singh requested review from dfokina and silvanocerza and removed request for a team September 30, 2024 05:03
@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Sep 30, 2024
@@ -23,7 +21,6 @@ def set_all_seeds(seed: int, deterministic_cudnn: bool = False) -> None:
:param deterministic_cudnn: Enable for full reproducibility when using CUDA. Caution: might slow down training.
"""
random.seed(seed)
np.random.seed(seed)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since numpy is no longer being used to generate random elements , we can safely remove this

@@ -5,7 +5,6 @@
import math
from typing import List

import numpy as np
Copy link
Contributor Author

@ajit97singh ajit97singh Sep 30, 2024

Choose a reason for hiding this comment

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

This particular file was importing numpy but no usage was found

@ajit97singh
Copy link
Contributor Author

@silvanocerza can you please review the changes

@coveralls
Copy link
Collaborator

coveralls commented Oct 2, 2024

Pull Request Test Coverage Report for Build 11404601053

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.001%) to 90.313%

Totals Coverage Status
Change from base Build 11400399865: -0.001%
Covered Lines: 7468
Relevant Lines: 8269

💛 - Coveralls

@ajit97singh
Copy link
Contributor Author

@silvanocerza just a heads up that all tests have passed

@anakin87 anakin87 self-assigned this Oct 18, 2024
@anakin87 anakin87 self-requested a review October 18, 2024 13:16
@anakin87 anakin87 removed their assignment Oct 18, 2024
Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

I felt free to remove numpy usage in some other places...

@anakin87 anakin87 merged commit 6cf13e8 into deepset-ai:main Oct 18, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove usage of numpy and use builtin math when possible
3 participants