Repository: alteryx/woodwork (python)

Issue:
Move LatLong and Ordinal logical type validation logic to LogicalType.validate methods
Currently, there is logical type validation for Ordinal and LatLong columns in `WoodworkColumnAccessor._validate_logical_type` (theres actually also a public `WoodworkColumnAccessor.validate_logical_type` to make things more confusing!). This method is called at Woodwork-initialization of a series (which makes them the only logical types that automatically have validation performed). 

We should move the validation logic onto the Logical type class under the public `validate` method so that our validation methods on column and table accessors can call them. 

We should also determine if we are okay with leaving just these two logical types' validation as an automatic part of columnd accessor initialization, and if we are okay with it, what makes these logical types different from the others?


Ordinal:
- logical type has a `Ordinal._validate_data` method that gets called in both `WoodworkColumnAccessor._validate_logical_type` and  `Ordinal.transform` (this mean that ordinal validation even happens at table accessor initialization)

LatLong:
- `_is_valid_latlong_series` utility exists that gets called in `WoodworkColumnAccessor._validate_logical_type`

Pull request description:
Move LatLong and Ordinal logical type validation logic to LogicalType…
- Move LatLong and Ordinal logical type validation logic to LogicalType.validate methods 
- Fixes #1305



Patch:
diff --git a/docs/source/release_notes.rst b/docs/source/release_notes.rst
index 0baf518..3fac52f 100644
--- a/docs/source/release_notes.rst
+++ b/docs/source/release_notes.rst
@@ -13,6 +13,7 @@ Future Release
         * Refactor serialization and deserialization for improved modularity (:pr:`1325`)
     * Documentation Changes
         * Update copy and paste button to remove syntax signs (:pr:`1313`)
+        * Move LatLong and Ordinal logical type validation logic to LogicalType.validate methods (:pr:`1315`)
         * Add backport release support (:pr:`1321`)
         * Add `get_subset_schema` to API reference (:pr:`1335`)
     * Testing Changes
diff --git a/woodwork/column_accessor.py b/woodwork/column_accessor.py
index 6d5b5a0..7ceefd2 100644
--- a/woodwork/column_accessor.py
+++ b/woodwork/column_accessor.py
@@ -11,16 +11,16 @@ from woodwork.accessor_utils import (
     init_series,
 )
 from woodwork.column_schema import ColumnSchema
-from woodwork.exceptions import ParametersIgnoredWarning, TypingInfoMismatchWarning
+from woodwork.exceptions import (
+    ParametersIgnoredWarning,
+    TypeValidationError,
+    TypingInfoMismatchWarning,
+)
 from woodwork.indexers import _iLocIndexer, _locIndexer
 from woodwork.logical_types import _NULLABLE_PHYSICAL_TYPES, LatLong, Ordinal
 from woodwork.statistics_utils import _get_box_plot_info_for_column
 from woodwork.table_schema import TableSchema
-from woodwork.utils import (
-    _get_column_logical_type,
-    _is_valid_latlong_series,
-    import_or_none,
-)
+from woodwork.utils import _get_column_logical_type, import_or_none
 
 dd = import_or_none("dask.dataframe")
 ks = import_or_none("databricks.koalas")
@@ -99,7 +99,17 @@ class WoodworkColumnAccessor:
             )
 
             if validate:
-                self._validate_logical_type(logical_type)
+                if isinstance(logical_type, (Ordinal, LatLong)):
+                    logical_type.validate(self._series)
+                else:
+                    valid_dtype = logical_type._get_valid_dtype(type(self._series))
+                    if valid_dtype != str(self._series.dtype):
+                        raise TypeValidationError(
+                            f"Cannot initialize Woodwork. Series dtype '{self._series.dtype}' is "
+                            f"incompatible with {logical_type} dtype. Try converting series "
+                            f"dtype to '{valid_dtype}' before initializing or use the "
+                            "woodwork.init_series function to initialize."
+                        )
 
             self._schema = ColumnSchema(
                 logical_type=logical_type,
@@ -305,28 +315,6 @@ class WoodworkColumnAccessor:
         # Directly return non-callable Series attributes
         return series_attr
 
-    def _validate_logical_type(self, logical_type):
-        """Validates that a logical type is consistent with the series dtype. Performs additional type
-        specific validation, as required."""
-        valid_dtype = logical_type._get_valid_dtype(type(self._series))
-        if valid_dtype != str(self._series.dtype):
-            raise ValueError(
-                f"Cannot initialize Woodwork. Series dtype '{self._series.dtype}' is "
-                f"incompatible with {logical_type} dtype. Try converting series "
-                f"dtype to '{valid_dtype}' before initializing or use the "
-                "woodwork.init_series function to initialize."
-            )
-
-        if isinstance(logical_type, Ordinal):
-            logical_type._validate_data(self._series)
-        elif isinstance(logical_type, LatLong):
-            if not _is_valid_latlong_series(self._series):
-                raise ValueError(
-                    "Cannot initialize Woodwork. Series does not contain properly formatted "
-                    "LatLong data. Try reformatting before initializing or use the "
-                    "woodwork.init_series function to initialize."
-                )
-
     @_check_column_schema
     def add_semantic_tags(self, semantic_tags):
         """Add the specified semantic tags to the set of tags.
@@ -438,6 +426,8 @@ class WoodworkColumnAccessor:
     @_check_column_schema
     def validate_logical_type(self, return_invalid_values=False):
         """Validates series data based on the logical type.
+        If a column's dtype does not match its logical type's required dtype,
+        will raise a TypeValidationError even when return_invalid_indices is True.
 
         Args:
             return_invalid_values (bool): Whether or not to return invalid data values
diff --git a/woodwork/logical_types.py b/woodwork/logical_types.py
index 09b8157..d7d523f 100644
--- a/woodwork/logical_types.py
+++ b/woodwork/logical_types.py
@@ -14,6 +14,7 @@ from woodwork.exceptions import (
 from woodwork.type_sys.utils import _get_specified_ltype_params
 from woodwork.utils import (
     _infer_datetime_format,
+    _is_valid_latlong_series,
     _reformat_to_latlong,
     camel_to_snake,
     import_or_none,
@@ -70,8 +71,15 @@ class LogicalType(object, metaclass=LogicalTypeMetaClass):
                 raise TypeConversionError(series, new_dtype, type(self))
         return series
 
-    def validate(self, series, return_invalid_values=True):
-        return
+    def validate(self, series, *args, **kwargs):
+        """Validates that a logical type is consistent with the series dtype. Performs additional type
+        specific validation, as required. When the series' dtype does not match the logical types' required dtype,
+        raises a TypeValidationError."""
+        valid_dtype = self._get_valid_dtype(type(series))
+        if valid_dtype != str(series.dtype):
+            raise TypeValidationError(
+                f"Series dtype '{series.dtype}' is incompatible with {self.type_string} dtype."
+            )
 
 
 class Address(LogicalType):
@@ -448,6 +456,16 @@ class LatLong(LogicalType):
 
         return super().transform(series)
 
+    def validate(self, series, return_invalid_values=False):
+        # TODO: we'll want to actually handle return_invalid_values in the ordinal and latlong logical types.
+        super().validate(series)
+        if not _is_valid_latlong_series(series):
+            raise TypeValidationError(
+                "Cannot initialize Woodwork. Series does not contain properly formatted "
+                "LatLong data. Try reformatting before initializing or use the "
+                "woodwork.init_series function to initialize."
+            )
+
 
 class NaturalLanguage(LogicalType):
     """Represents Logical Types that contain text or characters representing
@@ -502,7 +520,7 @@ class Ordinal(LogicalType):
     def __init__(self, order=None):
         self.order = order
 
-    def _validate_data(self, series):
+    def _validate_order_values(self, series):
         """Make sure order values are properly defined and confirm the supplied series
         does not contain any values that are not in the specified order values"""
         if self.order is None:
@@ -523,7 +541,7 @@ class Ordinal(LogicalType):
 
     def transform(self, series):
         """Validates the series and converts the dtype to match the logical type's if it is different."""
-        self._validate_data(series)
+        self._validate_order_values(series)
 
         typed_ser = super().transform(series)
         if pdtypes.is_categorical_dtype(typed_ser.dtype):
@@ -531,6 +549,11 @@ class Ordinal(LogicalType):
 
         return typed_ser
 
+    def validate(self, series, return_invalid_values=False):
+        # TODO: we'll want to actually handle return_invalid_values in the ordinal and latlong logical types.
+        super().validate(series)
+        self._validate_order_values(series)
+
 
 class PhoneNumber(LogicalType):
     """Represents Logical Types that contain numeric digits and characters
diff --git a/woodwork/table_accessor.py b/woodwork/table_accessor.py
index f30fecc..90c3ded 100644
--- a/woodwork/table_accessor.py
+++ b/woodwork/table_accessor.py
@@ -1117,6 +1117,8 @@ class WoodworkTableAccessor:
     @_check_table_schema
     def validate_logical_types(self, return_invalid_values=False):
         """Validates the dataframe based on the logical types.
+        If a column's dtype does not match its logical type's required dtype,
+        will raise a TypeValidationError even when return_invalid_indices is True.
 
         Args:
             return_invalid_values (bool): Whether or not to return invalid data values


Test patch:
diff --git a/woodwork/tests/accessor/test_column_accessor.py b/woodwork/tests/accessor/test_column_accessor.py
index ec46a64..f7893d5 100644
--- a/woodwork/tests/accessor/test_column_accessor.py
+++ b/woodwork/tests/accessor/test_column_accessor.py
@@ -159,7 +159,7 @@ def test_accessor_init_with_invalid_logical_type(sample_series):
         f"NaturalLanguage dtype. Try converting series dtype to '{correct_dtype}' before "
         "initializing or use the woodwork.init_series function to initialize."
     )
-    with pytest.raises(ValueError, match=error_message):
+    with pytest.raises(TypeValidationError, match=error_message):
         series.ww.init(logical_type=NaturalLanguage)
 
 
@@ -686,7 +686,7 @@ def test_latlong_init_error_with_invalid_series(latlongs):
         "LatLong data. Try reformatting before initializing or use the "
         "woodwork.init_series function to initialize."
     )
-    with pytest.raises(ValueError, match=error_message):
+    with pytest.raises(TypeValidationError, match=error_message):
         series.ww.init(logical_type="LatLong")
 
 
@@ -857,19 +857,54 @@ def test_schema_property(sample_series):
     assert changed_schema != sample_series.ww._schema
 
 
-@patch("woodwork.column_accessor.WoodworkColumnAccessor._validate_logical_type")
-def test_validation_methods_called_init(mock_validate_logical_type, sample_series):
-    assert not mock_validate_logical_type.called
+@patch("woodwork.logical_types.LogicalType.validate")
+def test_validation_methods_called_init(mock_validate, sample_series):
+    assert not mock_validate.called
 
     not_validated = sample_series.copy()
     not_validated.ww.init(validate=False)
 
-    assert not mock_validate_logical_type.called
+    assert not mock_validate.called
 
     validated = sample_series.copy()
     validated.ww.init(validate=True)
 
-    assert mock_validate_logical_type.called
+    assert not mock_validate.called
+    assert validated.ww == not_validated.ww
+    pd.testing.assert_series_equal(to_pandas(validated), to_pandas(not_validated))
+
+
+@patch("woodwork.logical_types.LogicalType.validate")
+def test_ordinal_validation_methods_called_init(mock_validate, sample_series):
+    assert not mock_validate.called
+
+    not_validated = sample_series.copy()
+    not_validated.ww.init(logical_type=Ordinal(order=["a", "b", "c"]), validate=False)
+
+    assert not mock_validate.called
+
+    validated = sample_series.copy()
+    validated.ww.init(logical_type=Ordinal(order=["a", "b", "c"]), validate=True)
+
+    assert mock_validate.called
+    assert validated.ww == not_validated.ww
+    pd.testing.assert_series_equal(to_pandas(validated), to_pandas(not_validated))
+
+
+@patch("woodwork.logical_types.LogicalType.validate")
+def test_latlong_validation_methods_called_init(mock_validate, latlong_df_pandas):
+    assert not mock_validate.called
+
+    latlong_series = latlong_df_pandas["null_latitude"]
+    not_validated = latlong_series.copy()
+    not_validated.ww.init(LatLong, validate=False)
+
+    assert not mock_validate.called
+
+    validated = latlong_series.copy()
+    validated.ww.init(LatLong, validate=True)
+
+    assert mock_validate.called
     assert validated.ww == not_validated.ww
     pd.testing.assert_series_equal(to_pandas(validated), to_pandas(not_validated))
 
diff --git a/woodwork/tests/accessor/test_table_accessor.py b/woodwork/tests/accessor/test_table_accessor.py
index b8796de..f7e2a5c 100644
--- a/woodwork/tests/accessor/test_table_accessor.py
+++ b/woodwork/tests/accessor/test_table_accessor.py
@@ -2986,7 +2986,19 @@ def test_nan_index_error(sample_df_pandas):
 
 def test_validate_logical_types(sample_df):
     df = sample_df[["email", "url", "age"]]
-    df.ww.init(logical_types={"email": "EmailAddress"})
+    if isinstance(df, pd.DataFrame):
+        df["ordinal"] = [18, 33, 33, 57]
+        df["latlong"] = [(1, 2), (3, 4), (5, 6), (8, 9)]
+
+        df.ww.init(
+            logical_types={
+                "email": "EmailAddress",
+                "ordinal": Ordinal(order=[18, 44, 33, 57]),
+                "latlong": "Latlong",
+            }
+        )
+    else:
+        df.ww.init(logical_types={"email": "EmailAddress"})
     assert df.ww.validate_logical_types() is None
 
     invalid_df = pd.DataFrame(
diff --git a/woodwork/tests/logical_types/test_logical_types.py b/woodwork/tests/logical_types/test_logical_types.py
index a1929c0..752847e 100644
--- a/woodwork/tests/logical_types/test_logical_types.py
+++ b/woodwork/tests/logical_types/test_logical_types.py
@@ -3,7 +3,7 @@ import re
 import pandas as pd
 import pytest
 
-from woodwork.accessor_utils import _is_koalas_series
+from woodwork.accessor_utils import _is_koalas_series, init_series
 from woodwork.exceptions import TypeConversionWarning, TypeValidationError
 from woodwork.logical_types import (
     URL,
@@ -141,6 +141,20 @@ def test_latlong_transform(latlong_df):
         pd.testing.assert_series_equal(actual, expected)
 
 
+def test_latlong_validate(latlong_df):
+    error_message = re.escape(
+        "Cannot initialize Woodwork. Series does not contain properly formatted "
+        "LatLong data. Try reformatting before initializing or use the "
+        "woodwork.init_series function to initialize."
+    )
+    latlong = LatLong()
+    series = latlong_df["tuple_ints"]
+    new_series = init_series(series, logical_type=LatLong)
+    latlong.validate(new_series)
+    with pytest.raises(TypeValidationError, match=error_message):
+        latlong.validate(series)
+
+
 def test_datetime_transform(datetimes):
     datetime = Datetime()
     for series in datetimes:
@@ -202,17 +216,45 @@ def test_ordinal_transform(sample_series):
         ordinal_incomplete_order.transform(sample_series)
 
 
+def test_ordinal_validate(sample_series):
+    series_type = str(type(sample_series))
+    dask = "dask" in series_type
+    koalas = "koalas" in series_type
+
+    if dask or koalas:
+        pytest.xfail(
+            "Fails with Dask and Koalas - ordinal data validation not supported"
+        )
+
+    ordinal_incomplete_order = Ordinal(order=["a", "b"])
+    error_msg = re.escape(
+        "Ordinal column sample_series contains values that are not "
+        "present in the order values provided: ['c']"
+    )
+
+    with pytest.raises(ValueError, match=error_msg):
+        ordinal_incomplete_order.validate(sample_series)
+
+    new_type = "string"
+    error_message = re.escape(
+        f"Series dtype '{new_type}' is incompatible with ordinal dtype."
+    )
+    with pytest.raises(TypeValidationError, match=error_message):
+        ordinal_incomplete_order.validate(sample_series.astype(new_type))
+
+
 def test_email_address_validate(sample_df):
-    series = sample_df["email"]
-    invalid_row = pd.Series({4: "bad_email"}, name="email")
+    email_address = EmailAddress()
+    dtype = email_address.primary_dtype
+    series = sample_df["email"].astype(dtype)
+    invalid_row = pd.Series({4: "bad_email"}, name="email").astype(dtype)
 
     if _is_koalas_series(series):
         invalid_row = ks.from_pandas(invalid_row)
 
-    email_address = EmailAddress()
     assert email_address.validate(series) is None
 
-    series = series.append(invalid_row)
+    series = series.append(invalid_row).astype(dtype)
     match = "Series email contains invalid email address values. "
     match += "The email_inference_regex can be changed in the config if needed."
 
@@ -220,21 +262,21 @@ def test_email_address_validate(sample_df):
         email_address.validate(series)
 
     actual = email_address.validate(series, return_invalid_values=True)
-    expected = pd.Series({4: "bad_email"}, name="email")
+    expected = pd.Series({4: "bad_email"}, name="email").astype(dtype)
     assert to_pandas(actual).equals(expected)
 
 
 def test_url_validate(sample_df):
-    series = sample_df["url"]
-    invalid_row = pd.Series({4: "bad_url"}, name="url")
-
+    logical_type = URL()
+    dtype = logical_type.primary_dtype
+    series = sample_df["url"].astype(dtype)
+    invalid_row = pd.Series({4: "bad_url"}, name="url").astype(dtype)
     if _is_koalas_series(series):
         invalid_row = ks.from_pandas(invalid_row)
 
-    logical_type = URL()
     assert logical_type.validate(series) is None
 
-    series = series.append(invalid_row)
+    series = series.append(invalid_row).astype(dtype)
     match = "Series url contains invalid url values. "
     match += "The url_inference_regex can be changed in the config if needed."
 
@@ -242,7 +284,7 @@ def test_url_validate(sample_df):
         logical_type.validate(series)
 
     actual = logical_type.validate(series, return_invalid_values=True)
-    expected = pd.Series({4: "bad_url"}, name="url")
+    expected = pd.Series({4: "bad_url"}, name="url").astype(dtype)
     assert to_pandas(actual).equals(expected)