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

Remove file loading for critical values #10

Merged
merged 1 commit into from
May 6, 2022

Conversation

prokolyvakis
Copy link
Contributor

@prokolyvakis prokolyvakis commented May 5, 2022

What?

  • Add a Python class Consts that wraps up all the constants used by the diptest.py module.
    • The Consts class is implemented in a way that forces the variables to stay constant.
  • Replace the np.loadtxt call and directly adding the aforementioned array in the Consts class.

Why?

Currently, the _CRIT_VALS numpy array is being read by calling the np.loadtxt method. Nevertheless, this adds a redundant call to the file system; in my laptop the reading op takes ~0.56 ms compared to the ~40ns when we directly add the array inside a Python module and import it.

How?

  • Remove dip_crit.txt.
  • Remove all the consts definitions from diptest.py module
  • Add a consts.py module that wraps up all the constants defined in the diptest.py module.
    • Add the values of the _CRIT_VALS explicitly inside the module.
    • Restrict all the class variables from being changed.

Testing?

The implementation has been tested locally in a Darwin platform and all the unittests pass with success.

@RUrlus RUrlus merged commit 2be8004 into RUrlus:stable May 6, 2022
@RUrlus
Copy link
Owner

RUrlus commented May 6, 2022

Hi @prokolyvakis, good idea! Thanks for the PR

@prokolyvakis prokolyvakis deleted the feature-remove-fs-reads branch May 6, 2022 08:50
@prokolyvakis
Copy link
Contributor Author

Hi @RUrlus! Thanks for accepting the PR!

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.

2 participants