Improve datetime handling in `ScalarInequality` and `ScalarRange` #853

pvk-developer · 2022-06-23T13:32:08Z

Resolves #819

codecov-commenter · 2022-06-23T13:59:36Z

Codecov Report

Merging #853 (be3e27c) into master (6939278) will increase coverage by 0.49%.
The diff coverage is 100.00%.

❗ Current head be3e27c differs from pull request most recent head 8594098. Consider uploading reports for the commit 8594098 to get more accurate results

@@            Coverage Diff             @@
##           master     #853      +/-   ##
==========================================
+ Coverage   67.65%   68.15%   +0.49%     
==========================================
  Files          38       38              
  Lines        2823     2867      +44     
==========================================
+ Hits         1910     1954      +44     
  Misses        913      913

Impacted Files	Coverage Δ
sdv/constraints/tabular.py	`99.74% <100.00%> (+0.01%)`	⬆️
sdv/constraints/utils.py	`100.00% <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 6939278...8594098. Read the comment docs.

amontanez24

Looking good! Mostly minor comments about restructuring

amontanez24 · 2022-06-23T19:37:38Z

sdv/constraints/tabular.py

@@ -424,10 +425,17 @@ def _validate_inputs(column_name, value, relation):
        if not (isinstance(value, (int, float)) or is_datetime_type(value)):
            raise ValueError('`value` must be a number or datetime.')

+        if is_datetime_type(value):


since this is called above can we capture the result in a variable?

amontanez24 · 2022-06-23T19:40:36Z

sdv/constraints/tabular.py

+            return cast_to_datetime64(value)
+
+        return value


I don't think the _validate_inputs method should return a value. If we want to group the logic we can make a parse_value method that calls _validate_inputs and then parses to datetime if necessary

sdv/constraints/tabular.py

amontanez24 · 2022-06-23T19:45:44Z

sdv/constraints/tabular.py

@@ -531,6 +541,11 @@ def reverse_transform(self, table_data):
            original_column = self._value - diff_column

        table_data[self._column_name] = pd.Series(original_column).astype(self._dtype)
+        if self._is_datetime and self._datetime_format:


is there ever a case where self._datetime_format is True, but self._is_datetime isn't? There shouldn't be right because the former is only set if the latter is True

If self._is_datetime is True but we are not able to detect the format of the date time, then we can't format with None.

I mean the other way around. Couldn't the if statement just be if self._datetime_format? Why do we need to check the self._is_datetime as well?

sdv/constraints/tabular.py

amontanez24 · 2022-06-23T19:54:12Z

Has the test coverage been updated? If not it seems like some cases are missing

fealho

Looking good, I just had a few questions + some typos.

fealho · 2022-06-23T19:32:58Z

sdv/constraints/tabular.py

@@ -531,6 +541,11 @@ def reverse_transform(self, table_data):
            original_column = self._value - diff_column

        table_data[self._column_name] = pd.Series(original_column).astype(self._dtype)
+        if self._is_datetime and self._datetime_format:


Is the self._datetime_format check really necessary? It's only False when the data is empty or filled with nans, but in those cases self._is_datetime should be False, right?

Sometimes self._datetime_format may not be learnt (it can return None) since the learning of the format does not support ms:

In [32]: detect_datetime_format('2021-02-02T00:00:00.000000') In [33]: detect_datetime_format('2021-02-02T00:00:00') Out[33]: '%Y-%m-%dT%H:%M:%S'

fealho · 2022-06-23T19:44:36Z

sdv/constraints/tabular.py


        if self._is_datetime:
-            table_data[self.column_name] = pd.to_datetime(data)
+            table_data[self._column_name] = pd.to_datetime(data)
+            if self._datetime_format:


Same as above, do we really need this check?

fealho · 2022-06-23T19:48:37Z

sdv/constraints/utils.py

+        ``numpy.datetime64`` value or values.
+    """
+    if isinstance(value, str):
+        value = pd.to_datetime(value).to_datetime64()


Why do we need to do to_datetime and to_datetime64?

I used it to ensure it ends up being numpy.datetime64[ns] that way all the datetimes are in the same magnitude.

fealho · 2022-06-23T19:48:57Z

sdv/constraints/utils.py

+
+
+def detect_datetime_format(value):
+    """Detect if possible the datetime format.


Docstring missing a word?

fealho · 2022-06-23T19:53:14Z

tests/unit/constraints/test_tabular.py

+    def test__validate_inputs_with_numerical_value(self):
+        """Test the ``_validate_inputs`` method.
+
+        Ensure the method crashes when the column name is not a string.


Incorrect, method doesn't crash.

fealho · 2022-06-23T19:55:04Z

tests/unit/constraints/test_tabular.py

+    def test__validate_inputs_with_datetime_value(self):
+        """Test the ``_validate_inputs`` method.
+
+        Ensure the method crashes when the column name is not a string.


Incorrect, method doesn't crash.

fealho · 2022-06-23T20:16:46Z

tests/unit/constraints/test_utils.py

+def test_is_datetime_type_with_datetime_str():
+    """Test the ``is_datetime_type`` function when an valid datetime string is passed.
+
+    Expect to return False when an int variable is passed.


Should be expected to return True.

fealho · 2022-06-23T20:17:10Z

tests/unit/constraints/test_utils.py

+    Input:
+    - string
+    Output:
+    - True


fealho · 2022-06-23T20:17:24Z

tests/unit/constraints/test_utils.py

+def test_is_datetime_type_with_invalid_str():
+    """Test the ``is_datetime_type`` function when an invalid string is passed.
+
+    Expect to return False when an int variable is passed.


Str, not int

fealho · 2022-06-23T20:28:02Z

tests/unit/constraints/test_tabular.py


        # Assert
-        pd.testing.assert_frame_equal(table_data, out)


Why did you delete this check?

amontanez24

Just a couple small comments and address Felipe's comment on the test file and then this is good to go!

amontanez24 · 2022-06-28T17:25:26Z

sdv/constraints/tabular.py

+        if value_is_datetime:
+            if not isinstance(value, str):


this can just be an and on one level right? ie

if value_is_datetime and not isinstance(value, str):

amontanez24 · 2022-06-28T19:16:40Z

sdv/constraints/tabular.py

            raise ValueError('`value` must be a number or datetime.')

+        if value_is_datetime not isinstance(value, str):


I think this should have an and in between

pvk-developer force-pushed the issue-819-improve-datetime-handling-in-scalarinequality-and-scalarrange branch from c5206b5 to 2a7b48d Compare June 23, 2022 13:49

pvk-developer marked this pull request as ready for review June 23, 2022 14:40

pvk-developer requested a review from a team as a code owner June 23, 2022 14:40

pvk-developer requested review from fealho and amontanez24 and removed request for a team June 23, 2022 14:40

amontanez24 requested changes Jun 23, 2022

View reviewed changes

fealho requested changes Jun 23, 2022

View reviewed changes

amontanez24 approved these changes Jun 28, 2022

View reviewed changes

pvk-developer force-pushed the issue-819-improve-datetime-handling-in-scalarinequality-and-scalarrange branch 2 times, most recently from 942bd90 to be3e27c Compare June 28, 2022 19:14

amontanez24 reviewed Jun 28, 2022

View reviewed changes

pvk-developer force-pushed the issue-819-improve-datetime-handling-in-scalarinequality-and-scalarrange branch from 2413f01 to 8594098 Compare June 28, 2022 19:29

amontanez24 approved these changes Jun 28, 2022

View reviewed changes

fealho self-requested a review June 28, 2022 20:34

fealho approved these changes Jun 28, 2022

View reviewed changes

pvk-developer added 8 commits June 29, 2022 10:56

Add better support for datetime

d282491

Update tests

8914047

Add missing if

35e98e4

Add utils tests

e771b57

Address comments

44b32b4

Fix docstring

ed382f1

Fix if

b2655bd

Remove handling strategy comments

aa33b3e

pvk-developer force-pushed the issue-819-improve-datetime-handling-in-scalarinequality-and-scalarrange branch from 8594098 to aa33b3e Compare June 29, 2022 08:57

pvk-developer merged commit c2c2c0d into master Jun 29, 2022

pvk-developer deleted the issue-819-improve-datetime-handling-in-scalarinequality-and-scalarrange branch June 29, 2022 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve datetime handling in `ScalarInequality` and `ScalarRange` #853

Improve datetime handling in `ScalarInequality` and `ScalarRange` #853

pvk-developer commented Jun 23, 2022

codecov-commenter commented Jun 23, 2022 •

edited

Loading

amontanez24 left a comment

amontanez24 Jun 23, 2022

amontanez24 Jun 23, 2022

amontanez24 Jun 23, 2022

pvk-developer Jun 27, 2022

amontanez24 Jun 27, 2022 •

edited

Loading

amontanez24 commented Jun 23, 2022

fealho left a comment

fealho Jun 23, 2022

pvk-developer Jun 27, 2022

fealho Jun 23, 2022

fealho Jun 23, 2022

pvk-developer Jun 27, 2022

fealho Jun 23, 2022

fealho Jun 23, 2022

fealho Jun 23, 2022

fealho Jun 23, 2022

fealho Jun 23, 2022

fealho Jun 23, 2022

fealho Jun 23, 2022

amontanez24 left a comment

amontanez24 Jun 28, 2022

amontanez24 Jun 28, 2022



		def detect_datetime_format(value):
		"""Detect if possible the datetime format.

		raise ValueError('`value` must be a number or datetime.')

		if value_is_datetime not isinstance(value, str):

Improve datetime handling in ScalarInequality and ScalarRange #853

Improve datetime handling in ScalarInequality and ScalarRange #853

Conversation

pvk-developer commented Jun 23, 2022

codecov-commenter commented Jun 23, 2022 • edited Loading

Codecov Report

amontanez24 left a 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

amontanez24 Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

amontanez24 commented Jun 23, 2022

fealho left a 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

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

amontanez24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Improve datetime handling in `ScalarInequality` and `ScalarRange` #853

Improve datetime handling in `ScalarInequality` and `ScalarRange` #853

codecov-commenter commented Jun 23, 2022 •

edited

Loading

amontanez24 Jun 27, 2022 •

edited

Loading