Added seed library variable #271

gautomdas · 2021-06-16T16:58:02Z

No description provided.

CLAassistant · 2021-06-16T16:58:07Z

All committers have signed the CLA.

dataprofiler/profilers/utils.py

JGSweets · 2021-06-16T17:02:18Z

dataprofiler/profilers/utils.py

+            seed_value = int(settings._seed)
+            rng = np.random.default_rng(seed_value)
+        except ValueError as e:
+            warnings.warn("Seed should be an integer", RuntimeWarning)


should reference that it should be >= 0

JGSweets · 2021-06-16T17:03:02Z

dataprofiler/profilers/utils.py

+    if settings._seed is not None:
+        try:
+            seed_value = int(settings._seed)
+            rng = np.random.default_rng(seed_value)


in this case, probably don't need to set an intermediate variable.

seed_value

also, we should assume that settings._seed is in its correct form. either None or int >= 0

JGSweets · 2021-06-16T17:03:46Z

dataprofiler/tests/profilers/utils.py

@@ -3,6 +3,7 @@
 import random

 import numpy as np
+from dataprofiler import settings


as above with the imports, should be a space between these imports since numpy is a 3rd party lib

JGSweets · 2021-06-16T17:04:35Z

dataprofiler/tests/profilers/utils.py

-    np.random.seed(seed)
-    random.seed(seed)
+    settings._seed = seed


this should also set the seed for np.random and random.seed in addition to the settings

or maybe we should remove this and the set_seed func in the init should set these as well.

In this case, we would likely move the set_seed into its own file and import set_seed from the file so that it could be accessed directly via dp.set_seed

@granteden @ChrisWallace2020 thoughts?

e.g. a utils

also utils shouldn't directly alter the _seed but utilize the dp.set_seed

I think it should be a solution in one place, rather than top level in init and here. Because if you are just setting _seed here then what is the use case for the one in __init__?

actually, now that i think about it, i think we leave it out of __init__ reason being, the user can choose to specify those externally. If we alter it globally, we could be altering it for someone else.

ChrisWallace2020 · 2021-06-16T19:44:21Z

dataprofiler/__init__.py

@@ -22,3 +23,6 @@
        '\tsudo apt-get -y install libsnappy-dev`\n',
        ImportWarning
    )
+
+def set_seed(seed=None):
+    settings._seed = seed


Need a blank new line at the end of the file

ChrisWallace2020 · 2021-06-16T19:47:21Z

dataprofiler/settings.py

@@ -0,0 +1 @@
+_seed = None


Need an empty new line at the end of the file

dataprofiler/__init__.py

… main

JGSweets · 2021-06-17T19:36:15Z

dataprofiler/__init__.py

+
+def set_seed(seed=None):
+    # also check it's an integer
+    if seed is not None and (seed < 0 or int(seed)!=float(seed)):


if seed were a {} it would not have a < operator. hence i think we need to alter this to check specifically that it is an int
e.g. if seed is not None and (not isinstance(seed, int) or seed < 0):

JGSweets · 2021-06-17T19:36:58Z

dataprofiler/__init__.py

+    if seed is not None and (seed < 0 or int(seed)!=float(seed)):
+        raise ValueError("Seed should be a non-negative integer.")
+    settings._seed = seed
+


this line might have whitespaces in it

JGSweets · 2021-06-17T19:37:23Z

dataprofiler/profilers/utils.py

-
-    rng = np.random.default_rng()
+
+    rng = np.random.default_rng(int(settings._seed))


since we already check in dp.set_seed, remove the int from here

dataprofiler/profilers/utils.py

JGSweets · 2021-06-17T19:52:13Z

dataprofiler/__init__.py

@@ -7,6 +7,7 @@
                                    UnstructuredDataLabeler
 from .validators.base_validators import Validator
 from .version import __version__
+from . import settings



two spaces between imports and code

dataprofiler/profilers/utils.py

ChrisWallace2020 · 2021-06-21T15:44:47Z

dataprofiler/tests/labelers/utils.py

+
+def set_seed(seed=None):
+    """
+    Sets the see for all possible random state libraries


typo, I'm guessing this is supposed to be "sets the seed"

Its also replicated in the other set_seed function

ChrisWallace2020 · 2021-06-21T15:46:21Z

dataprofiler/tests/profilers/utils.py

@@ -13,6 +15,7 @@ def set_seed(seed=None):
    """
    np.random.seed(seed)
    random.seed(seed)
+    dp.set_seed(seed)


Why is this written for profilers and labelers when its the same function? Is there a reason this isn't more centrally located?

grant-eden · 2021-06-21T15:47:51Z

dataprofiler/profilers/utils.py

-    rng = np.random.default_rng()
-    if 'DATAPROFILER_SEED' in os.environ:
+
+    rng = np.random.default_rng(settings._seed)


Did we want to make this so that there is only want rng call instead of potentially 2?

ChrisWallace2020 · 2021-06-21T15:50:47Z

dataprofiler/tests/labelers/utils.py

+    np.random.seed(seed)
+    random.seed(seed)


Is there a reason these things aren't also set in the top level __init__ function?

Then these test utils just call that one? I understand this was a whole discussion previously, I am just unaware of the conclusion and the structure that was decided upon.

We don't want to globally set the seed for other libraries.

* Added seed library var * Checking testing environment * Added tests and removed unnecessary seed calls * If user sets seed don't look for one in the environment * Added file changes * Made small changes

Added seed library var

9fd402a

gautomdas requested review from AnhTruong, ChrisWallace2020, grant-eden, JGSweets and lettergram as code owners June 16, 2021 16:58

JGSweets reviewed Jun 16, 2021

View reviewed changes

dataprofiler/profilers/utils.py Show resolved Hide resolved

JGSweets reviewed Jun 16, 2021

View reviewed changes

JGSweets enabled auto-merge (squash) June 16, 2021 17:07

ChrisWallace2020 reviewed Jun 16, 2021

View reviewed changes

dataprofiler/settings.py Outdated

@@ -0,0 +1 @@

_seed = None

Copy link

Contributor

ChrisWallace2020 Jun 16, 2021

Choose a reason for hiding this comment

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

Need an empty new line at the end of the file

JGSweets reviewed Jun 16, 2021

View reviewed changes

dataprofiler/__init__.py Show resolved Hide resolved

Checking testing environment

818cde4

auto-merge was automatically disabled June 17, 2021 16:41
Head branch was pushed to by a user without write access

Merge branch 'main' into main

6c8054d

JGSweets enabled auto-merge (squash) June 17, 2021 19:04

gautomdas added 2 commits June 17, 2021 15:25

Added tests and removed unnecessary seed calls

b5ba666

Merge branch 'main' of https://github.com/gautomdas/DataProfiler into…

5dc43ac

… main

auto-merge was automatically disabled June 17, 2021 19:27
Head branch was pushed to by a user without write access

JGSweets enabled auto-merge (squash) June 17, 2021 19:31

JGSweets reviewed Jun 17, 2021

View reviewed changes

dataprofiler/profilers/utils.py Outdated Show resolved Hide resolved

If user sets seed don't look for one in the environment

8515d7e

auto-merge was automatically disabled June 17, 2021 19:45
Head branch was pushed to by a user without write access

JGSweets reviewed Jun 17, 2021

View reviewed changes

dataprofiler/profilers/utils.py Show resolved Hide resolved

JGSweets added High Priority Dramatic improvement, inaccurate calculation(s) or bug / feature making the library unusable New Feature A feature addition not currently in the library labels Jun 17, 2021

Added file changes

e6eb74d

JGSweets enabled auto-merge (squash) June 21, 2021 15:27

JGSweets previously approved these changes Jun 21, 2021

View reviewed changes

ChrisWallace2020 reviewed Jun 21, 2021

View reviewed changes

grant-eden reviewed Jun 21, 2021

View reviewed changes

ChrisWallace2020 reviewed Jun 21, 2021

View reviewed changes

Made small changes

c101833

auto-merge was automatically disabled June 21, 2021 16:02
Head branch was pushed to by a user without write access

gautomdas dismissed JGSweets’s stale review via c101833 June 21, 2021 16:02

JGSweets enabled auto-merge (squash) June 21, 2021 16:05

JGSweets approved these changes Jun 21, 2021

View reviewed changes

ChrisWallace2020 approved these changes Jun 21, 2021

View reviewed changes

JGSweets merged commit 5e0fb30 into capitalone:main Jun 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added seed library variable #271

Added seed library variable #271

gautomdas commented Jun 16, 2021

CLAassistant commented Jun 16, 2021 •

edited

Loading

JGSweets Jun 16, 2021 •

edited

Loading

JGSweets Jun 16, 2021

JGSweets Jun 16, 2021

JGSweets Jun 16, 2021 •

edited

Loading

JGSweets Jun 16, 2021

JGSweets Jun 16, 2021

JGSweets Jun 16, 2021

JGSweets Jun 16, 2021 •

edited

Loading

JGSweets Jun 16, 2021

JGSweets Jun 16, 2021

ChrisWallace2020 Jun 16, 2021

JGSweets Jun 16, 2021

ChrisWallace2020 Jun 16, 2021

ChrisWallace2020 Jun 16, 2021

JGSweets Jun 17, 2021

JGSweets Jun 17, 2021

JGSweets Jun 17, 2021 •

edited

Loading

JGSweets Jun 17, 2021

ChrisWallace2020 Jun 21, 2021

ChrisWallace2020 Jun 21, 2021

ChrisWallace2020 Jun 21, 2021

grant-eden Jun 21, 2021

ChrisWallace2020 Jun 21, 2021

ChrisWallace2020 Jun 21, 2021

JGSweets Jun 21, 2021


		rng = np.random.default_rng()

		rng = np.random.default_rng(int(settings._seed))

Added seed library variable #271

Added seed library variable #271

Conversation

gautomdas commented Jun 16, 2021

CLAassistant commented Jun 16, 2021 • edited Loading

JGSweets Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JGSweets Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JGSweets Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JGSweets Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Jun 16, 2021 •

edited

Loading

JGSweets Jun 16, 2021 •

edited

Loading

JGSweets Jun 16, 2021 •

edited

Loading

JGSweets Jun 16, 2021 •

edited

Loading

JGSweets Jun 17, 2021 •

edited

Loading