search

deephaven / deephaven-core

Deephaven Community Core
Other
257 stars 80 forks source link

fix: Add numpy as a required dependency for py client #6308

Closed jmao-denver closed 3 weeks ago

jmao-denver commented 3 weeks ago

and clean up setup.py a bit

Fixes #6304

jmao-denver commented 3 weeks ago

@devinrsmith wondered if we should make numpy a soft (optional) dependency. I guess we could use a pattern like the following but the body of unique also references specific numpy types, which requires more code branches and I don't think it is worth the effort.

if np is None:
    Sentinel_Type = Union[str, bool]
else:
    Sentinel_Type = Union[np.number, str, bool]

def unique(cols: Union[str, List[str]] = None, include_nulls: bool = False,
           non_unique_sentinel: Sentinel_Type = None) -> Aggregation:
rcaudy commented 3 weeks ago

@devinrsmith wondered if we should make numpy a soft (optional) dependency. I guess we could use a pattern like the following but the body of unique also references specific numpy types, which requires more code branches and I don't think it is worth the effort.

if np is None:
    Sentinel_Type = Union[str, bool]
else:
    Sentinel_Type = Union[np.number, str, bool]

def unique(cols: Union[str, List[str]] = None, include_nulls: bool = False,
           non_unique_sentinel: Sentinel_Type = None) -> Aggregation:

Is this the only feature we require numpy for? If so, I'm OK with the optional route.

jmao-denver commented 3 weeks ago
  1. I see the reason for making numpy optional, however the ROI isn't high considering the changes we need to make to the code and the doc for such a common lib
  2. Yes, TypeAlias or just type (3.12+) is the way to go for readability.