respond to code review comments

Author: ajpotts

arkouda/logger.py

@@ -71,7 +71,7 @@
     Logger,
     StreamHandler,
 )
-from typing import List, Optional, cast
+from typing import List, Optional, Union, cast
 
 from typeguard import typechecked
 
@@ -130,6 +130,11 @@ class LogLevel(Enum):
 """
 
 
+# sentinel
+_UNSET = object()
+_UnsetType = object  # just for readability in the union
+
+
 class ArkoudaLogger(Logger):
     DEFAULT_LOG_FORMAT = "[%(name)s] Line %(lineno)d %(levelname)s: %(message)s"
 
@@ -147,7 +152,7 @@ class ArkoudaLogger(Logger):
     def __init__(
         self,
         name: str,
-        log_level: LogLevel = LogLevel.INFO,
+        log_level: Union[LogLevel, _UnsetType] = _UNSET,  # sentinel means "not provided"
         handlers: Optional[List[Handler]] = None,
         log_format: Optional[str] = "[%(name)s] Line %(lineno)d %(levelname)s: %(message)s",
         **kwargs,
@@ -200,25 +205,37 @@ def __init__(
         the individual handlers.
 
         """
+        # --- log_level alias handling ---
         if "logLevel" in kwargs:
-            if log_level is not None:
+            if log_level is not _UNSET:
                 raise TypeError("Pass only one of 'logLevel' or 'log_level'")
             warnings.warn(
                 "'logLevel' is deprecated; use 'log_level' instead",
                 DeprecationWarning,
                 stacklevel=2,
             )
             log_level = kwargs.pop("logLevel")
+        elif log_level is _UNSET:
+            log_level = LogLevel.INFO
 
+        # --- log_format alias handling ---
         if "logFormat" in kwargs:
-            if log_format is not None:
+            if log_format is not _UNSET:
                 raise TypeError("Pass only one of 'logFormat' or 'log_format'")
             warnings.warn(
                 "'logFormat' is deprecated; use 'log_format' instead",
                 DeprecationWarning,
                 stacklevel=2,
             )
             log_format = kwargs.pop("logFormat")
+        elif log_format is _UNSET:
+            log_format = "[%(name)s] Line %(lineno)d %(levelname)s: %(message)s"
+
+        if not isinstance(log_level, LogLevel):
+            raise TypeError("log_level must be a LogLevel")
+
+        if not isinstance(log_format, str):
+            raise TypeError("log_format must be a str")
 
         if kwargs:
             raise TypeError(f"Unexpected keyword argument(s): {', '.join(kwargs)}")
@@ -276,7 +293,7 @@ def enableVerbose(self) -> None:
         self.changeLogLevel(LogLevel.DEBUG)
 
     @typechecked
-    def disableVerbose(self, log_level: LogLevel = LogLevel.INFO, **kwargs) -> None:
+    def disableVerbose(self, log_level: object = _UNSET, **kwargs) -> None:
         """
         Disables verbose output.
 
@@ -293,20 +310,25 @@ def disableVerbose(self, log_level: LogLevel = LogLevel.INFO, **kwargs) -> None:
         ------
         TypeError
             Raised if log_level is not a LogLevel enum
-
         """
         if "logLevel" in kwargs:
-            if log_level is not None:
+            if log_level is not _UNSET:
                 raise TypeError("Pass only one of 'logLevel' or 'log_level'")
             warnings.warn(
                 "'logLevel' is deprecated; use 'log_level' instead",
                 DeprecationWarning,
                 stacklevel=2,
             )
             log_level = kwargs.pop("logLevel")
+        elif log_level is _UNSET:
+            log_level = LogLevel.INFO
 
         if kwargs:
             raise TypeError(f"Unexpected keyword argument(s): {', '.join(kwargs)}")
+
+        if not isinstance(log_level, LogLevel):
+            raise TypeError("log_level must be a LogLevel")
+
         self.changeLogLevel(log_level)
 
     @typechecked
@@ -338,62 +360,64 @@ def getHandler(self, name: str) -> Handler:
         raise ValueError(f"The name {name} does not match any handler")
 
 
+_UNSET = object()
+
+
 @typechecked
 def getArkoudaLogger(
     name: str,
     handlers: Optional[List[Handler]] = None,
-    log_format: Optional[str] = ArkoudaLogger.DEFAULT_LOG_FORMAT,
-    log_level: Optional[LogLevel] = None,
+    log_format: object = _UNSET,
+    log_level: object = _UNSET,
     **kwargs,
 ) -> ArkoudaLogger:
-    """
-    Instantiate an ArkoudaLogger that retrieves the logging level from ARKOUDA_LOG_LEVEL env variable.
-
-    Parameters
-    ----------
-    name : str
-        The name of the ArkoudaLogger
-    handlers : List[Handler]
-        A list of logging.Handler objects, if None, a list consisting of
-        one StreamHandler named 'console-handler' is generated and configured
-    log_format : str
-        The format for log messages, defaults to the following format:
-        '[%(name)s] Line %(lineno)d %(levelname)s: %(message)s'
-
-    Returns
-    -------
-    ArkoudaLogger
-
-    Raises
-    ------
-    TypeError
-        Raised if either name or log_format is not a str object or if handlers
-        is not a list of str objects
-
-    Notes
-    -----
-    Important note: if a list of 1..n logging.Handler objects is passed in, and
-    dynamic changes to 1..n handlers is desired, set a name for each Handler
-    object as follows: handler.name = <desired name>, which will enable retrieval
-    and updates for the specified handler.
-
-    """
+    # --- deprecated alias: logFormat -> log_format ---
     if "logFormat" in kwargs:
-        if log_format is not None and kwargs["logFormat"] is not None:
-            raise TypeError("Pass only one of logFormat or log_format")
-        warnings.warn("logFormat is deprecated; use log_format", DeprecationWarning, stacklevel=2)
+        if log_format is not _UNSET:
+            raise TypeError("Pass only one of 'logFormat' or 'log_format'")
+        warnings.warn(
+            "'logFormat' is deprecated; use 'log_format' instead",
+            DeprecationWarning,
+            stacklevel=2,
+        )
         log_format = kwargs.pop("logFormat")
 
+    # --- deprecated alias: logLevel -> log_level ---
     if "logLevel" in kwargs:
-        if log_level is not None and kwargs["logLevel"] is not None:
-            raise TypeError("Pass only one of logLevel or log_level")
-        warnings.warn("logLevel is deprecated; use log_level", DeprecationWarning, stacklevel=2)
+        if log_level is not _UNSET:
+            raise TypeError("Pass only one of 'logLevel' or 'log_level'")
+        warnings.warn(
+            "'logLevel' is deprecated; use 'log_level' instead",
+            DeprecationWarning,
+            stacklevel=2,
+        )
         log_level = kwargs.pop("logLevel")
 
-    if not log_level:
-        log_level = LogLevel(os.getenv("ARKOUDA_LOG_LEVEL", LogLevel("INFO")))
+    if kwargs:
+        raise TypeError(f"Unexpected keyword argument(s): {', '.join(kwargs)}")
 
-    logger = ArkoudaLogger(name=name, handlers=handlers, log_format=log_format, log_level=log_level)
+    # defaults
+    if log_format is _UNSET:
+        log_format = ArkoudaLogger.DEFAULT_LOG_FORMAT
+    if log_level is _UNSET:
+        env_val = os.getenv("ARKOUDA_LOG_LEVEL", "INFO")
+        try:
+            log_level = LogLevel(env_val)
+        except Exception as e:
+            raise ValueError(f"Invalid ARKOUDA_LOG_LEVEL={env_val!r}") from e
+
+    # validate types (since we used object for sentinel)
+    if not isinstance(log_format, str):
+        raise TypeError("log_format must be a str")
+    if not isinstance(log_level, LogLevel):
+        raise TypeError("log_level must be a LogLevel")
+
+    logger = ArkoudaLogger(
+        name=name,
+        handlers=handlers,
+        log_format=log_format,
+        log_level=log_level,
+    )
     loggers[logger.name] = logger
     return logger
 
@@ -437,8 +461,11 @@ def enableVerbose() -> None:
         logger.enableVerbose()
 
 
+_UNSET = object()
+
+
 @typechecked
-def disableVerbose(log_level: LogLevel = LogLevel.INFO, **kwargs) -> None:
+def disableVerbose(log_level: object = _UNSET, **kwargs) -> None:
     """
     Disables verbose logging.
 
@@ -448,27 +475,31 @@ def disableVerbose(log_level: LogLevel = LogLevel.INFO, **kwargs) -> None:
     Parameters
     ----------
     log_level : LogLevel
-        The new log level, defaultts to LogLevel.INFO
+        The new log level, defaults to LogLevel.INFO
 
     Raises
     ------
     TypeError
         Raised if log_level is not a LogLevel enum
-
     """
     if "logLevel" in kwargs:
-        if log_level is not None:
+        if log_level is not _UNSET:
             raise TypeError("Pass only one of 'logLevel' or 'log_level'")
         warnings.warn(
             "'logLevel' is deprecated; use 'log_level' instead",
             DeprecationWarning,
             stacklevel=2,
         )
         log_level = kwargs.pop("logLevel")
+    elif log_level is _UNSET:
+        log_level = LogLevel.INFO
 
     if kwargs:
         raise TypeError(f"Unexpected keyword argument(s): {', '.join(kwargs)}")
 
+    if not isinstance(log_level, LogLevel):
+        raise TypeError("log_level must be a LogLevel")
+
     for logger in loggers.values():
         logger.disableVerbose(log_level)
 

arkouda/numpy/pdarraysetops.py

@@ -722,7 +722,7 @@ def intersect1d(
             ub = bg.unique_keys
         else:
             ua = ar1
-            ub = ar1
+            ub = ar2
 
         # Key for deinterleaving result
         isa = concatenate(
@@ -823,9 +823,9 @@ def setdiff1d(ar1: groupable, ar2: groupable, assume_unique: bool = False) -> Un
             )
             return create_pdarray(cast(str, rep_msg))
         if not assume_unique:
-            a = cast(pdarray, unique(ar1))
-            b = cast(pdarray, unique(ar2))
-        x = a[in1d(a, b, invert=True)]
+            ar1 = cast(pdarray, unique(ar1))
+            ar2 = cast(pdarray, unique(ar2))
+        x = ar1[in1d(ar1, ar2, invert=True)]
         return x[argsort(x)]
     elif (isinstance(ar1, list) or isinstance(ar1, tuple)) and (
         isinstance(ar2, list) or isinstance(ar2, tuple)

arkouda/pandas/categorical.py

@@ -49,6 +49,7 @@
 
 import itertools
 import json
+import warnings
 
 from collections import defaultdict
 from typing import (
@@ -241,6 +242,22 @@ def __init__(self, values, **kwargs) -> None:
         self.dtype = akdtype(str_)
         self.registered_name: Optional[str] = None
 
+    @property
+    def NAvalue(self):
+        """
+        Deprecated alias for `na_value`.
+
+        Returns
+        -------
+        Same value as `na_value`.
+        """
+        warnings.warn(
+            "Categorical.NAvalue is deprecated; use Categorical.find_na instead.",
+            DeprecationWarning,
+            stacklevel=2,
+        )
+        return self.na_value
+
     @property
     def nbytes(self):
         """

tests/pandas/categorical_test.py

@@ -637,3 +637,8 @@ def test_copy_empty_categorical(self):
             if a is not None:
                 assert a is not b
                 assert_equal(a, b)
+
+    def test_categorical_NAvalue_deprecated_returns_find_na(self):
+        cat = Categorical(ak.array(["a", "b"]))
+        with pytest.deprecated_call():
+            assert cat.NAvalue == cat.na_value