Add details to expectations for scalars #308

MarcoGorelli · 2023-10-31T16:56:11Z

closes #305

kkraus14 · 2023-10-31T19:20:29Z

I think we need to have more details than this. I.E. is inheriting from float / int / bool required? Does isinstance(..., float) return True? Does the scalar need to support pickle, if so can it be namespace specific or does it need to return general Python scalars to work cross namespace? Which methods need to be supported vs which dont?

MarcoGorelli · 2023-11-01T17:22:58Z

I don't really have to strong an opinion on these. I can make a list of the methods which need to be supported, do you have preferences on the rest?

kkraus14 · 2023-11-01T19:57:43Z

I don't really have to strong an opinion on these. I can make a list of the methods which need to be supported, do you have preferences on the rest?

I'm still generally -1 on ducktyping Python scalars because of the messiness that will come when doing things like LIST / STRUCT / DECIMAL types where I'd be in much stronger favor of having a Scalar class that can have a much smaller API area that implementations need to conform to.

MarcoGorelli · 2023-11-01T20:49:32Z

agree - besides, the longer users stay within classes defined by the standard, the longer they know exactly what to expect

will update then, thanks

MarcoGorelli · 2023-11-09T14:37:39Z

I've added a list of required methods

MarcoGorelli · 2023-11-09T18:13:30Z

spec/design_topics/python_builtin_types.md

+- `__int__`
+- `__float__`


punt on these

MarcoGorelli · 2023-11-10T07:43:09Z

I've updated to add a Scalar class, to note that __bool__ may force compute (or raise, implementation-dependent), for now leaving out __int__ / __float__ / any other method to get a Python scalar out

OK to move forwards here?

MarcoGorelli · 2023-11-14T09:01:22Z

I think everyone agreed with at least the content of this PR

There is further discussion to be had, but I think we can defer it until later, I suggest we start by merging the part which we agree on

kkraus14 · 2023-11-10T18:41:45Z

spec/API_specification/dataframe_api/scalar_object.py

+__all__ = ['Scalar']
+
+
+class Scalar(Protocol):


I think we need a dtype property similar to what we have with Columns?

~~to be honest I'd be fine with departing completely from the idea of ducktyped Python scalars and just adding extra things (like dtype or persist) if they're useful~~

~~let's discuss this part in the next call~~

this would mean that Python scalars would no longer implement the Scalar Protocol

kkraus14 · 2023-11-10T18:43:42Z

spec/API_specification/dataframe_api/scalar_object.py

+__all__ = ['Scalar']
+
+
+class Scalar(Protocol):


Is there a clean way from a typing perspective to allow people to pass either Scalar objects or typical Python scalars that we can handle constructing Scalars from without issue? Without that I think the API will be a bit of a pain with people having to sprinkle Scalar(...) or Scalar.from_py(...) all over.

We should also have some way to go from Python scalars to these Scalar objects.

Yes, I've added an example (spec/API_specification/examples/05_scalars_example.py)

Python scalars do in fact implement the Scalar protocol, so they can be passed without any issue

I think this is an argument against adding dtype and other methods to our scalars which aren't supported by Python ones

Python scalars do in fact implement the Scalar protocol, so they can be passed without any issue

at least, Python floats do

trying to do fill_null('foo') wouldn't be fine

I'm starting to see the writing on the wall for FloatScalar / IntScalar / StringScalar ...

I imagine Scalars would be typed following the Column dtypes, which then allows us to define a consistent set of type handling and type promotion rules. Otherwise, if Scalars are not typed similarly to Columns, you may need to introspect the value of a Scalar for example in calling fill_null against an Int8 dtype column with an IntScalar where the value is above what is supported by Int8.

kkraus14 · 2023-11-10T18:47:13Z

spec/API_specification/dataframe_api/typing.py

 # null is a special object which represents a missing value.
 # It is not valid as a type.
 NullType = Any


I wonder if we can just have the special NULL scalar singleton be of type Scalar instead of a special type here? Not sure what NULL.dtype would yield though since we don't have a null or empty dtype

yes, brilliant point, thanks

I guess it could just return None?

from #308 (comment) , I think we may need to end up with NullScalar

kkraus14 · 2023-11-14T20:33:24Z

spec/design_topics/python_builtin_types.md

@@ -18,7 +18,7 @@ class DataFrame:
        ...

 class Column:
-    def mean(self, skip_nulls: bool = True) -> float | NullType:
+    def mean(self, skip_nulls: bool = True) -> Scalar | NullType:


I think it's weird that this can return either a Scalar which is a protocol or a NullType which doesn't guarantee the same interface. It makes it hard to guarantee the ability to nicely method chain here.

thanks, you're right - it should just be -> Scalar, correct? Because Scalar could be backed by a null value

if indeed we did go with #308 (comment), then Scalar could be a union of FloatScalar, IntScalar, NullScalar, ...

I don't know if we would want Scalar to be type erased similar to Column

Could you clarify please? You could do isinstance(value, namespace.FloatScalar) to check the dtype

Apologies, we're aligned here. We have different classes per type as opposed to just a top level Scalar class where type specific functionality is hidden underneath the class (similar to our Column class).

MarcoGorelli · 2023-11-15T13:33:00Z

If we add dtype / persist to the Scalar protocol, but Python scalars will no longer implement that protocol. But maybe that's OK

All we need is some separate type hints:

BoolScalar: either bool or Scalar
FloatScalar: either float or Scalar
...

Then:

return values will typically be typed as Scalar
arguments where the value can either be Scalar or PythonScalar can be typed as AnyScalar

kkraus14 · 2023-11-15T20:59:14Z

spec/API_specification/dataframe_api/typing.py

@@ -169,15 +167,31 @@ def __column_consortium_standard__(
        ...


+PythonScalar = Union[str, int, float, bool]


Need date and datetime values in here as well as DateScalar and DatetimeScalar I believe.

there's already namespace.date to create a scalar which can be used for date columns (e.g. see how it's used in TPCH Q1:

mask = lineitem.col("l_shipdate") <= namespace.date(1998, 9, 2)

), I think that's enough?

kkraus14 · 2023-11-15T21:38:40Z

spec/API_specification/dataframe_api/typing.py

+NumericScalar = Union[FloatScalar, IntScalar]
+StringScalar = Union[str, Scalar]
+AnyScalar = Union[PythonScalar, Scalar]
+NullType = Namespace.NullType


Is this a null scalar singleton or a null dtype or something else? If it's a null scalar singleton then I think it needs to be added to bunch of these Union types.

kkraus14 · 2023-11-15T21:41:25Z

spec/API_specification/dataframe_api/typing.py

+PythonScalar = Union[str, int, float, bool]
+BoolScalar = Union[bool, Scalar]
+FloatScalar = Union[float, Scalar]
+IntScalar = Union[int, Scalar]


Without Int8Scalar, Int16Scalar, Int32Scalar, I think we need to do value introspection to figure out how to handle type promotion or throwing or whatnot.

Should we have a Scalar class for each dtype?

These are just type hints

The idea is:

Scalar is the Standard class for scalars. It has dtype and persist methods

FloatScalar is just for type hint purposes and means "either a Python float or a standard Scalar

I could simplify and just have PythonScalar and Scaler, this is becoming too complex

MarcoGorelli · 2023-11-16T09:06:37Z

Have updated. We now have:

Scalar: class backed by a scalar, which allows data to reside on GPU or stay lazy. Has dtype property and persist method
AnyScalar: type hint meaning "either a Python scalar, or a Scalar`. This makes explicit that both
```
df: DataFrame
df > 3  # supported
df > df.col('a').mean()  # also supported
```
are supported (3 is a Python scalar, df.col('a').mean() is a Scalar)

null is just a singleton null value. It is not a Scalar, as it does not have a dtype defined. So, for fill_nan, the fill value can be float | NullType | Scalar, meaning you can do:

df: DataFrame
ns = df.__dataframe_namespace__()
df.col('a').fill_nan(3.)  # supported (`float`)
df.col('a').fill_nan(ns.null)  # also supported (`NullType`)
df.col('a').fill_nan(df.col('b').mean())  # also supported (`Scalar`)

From the discussion in #294, I've also added Scalar.persist, which seems very much needed.
If people are OK with this design, then I think #294 will finally be ready (sorry @cbourjau it's taken so long, and thanks for all the discussion which your example has spurred!)

MarcoGorelli · 2023-11-16T18:33:58Z

thanks for your review! leaving open a bit, if others have objections please speak up - else, really excited to be moving forwards, so glad we've found common ground on the design here

note what may raise

751a131

MarcoGorelli force-pushed the expand-on-scalars branch from 90f1645 to 834e978 Compare October 31, 2023 16:56

MarcoGorelli requested review from kkraus14 and cbourjau October 31, 2023 16:57

MarcoGorelli force-pushed the expand-on-scalars branch from 834e978 to 751a131 Compare October 31, 2023 16:59

MarcoGorelli requested a review from rgommers October 31, 2023 19:13

MarcoGorelli added the API design label Nov 1, 2023

MarcoGorelli added 8 commits November 7, 2023 11:52

Merge remote-tracking branch 'upstream/main' into expand-on-scalars

7cb90ea

list required methods

fc65648

add scalar class

7c24afd

reword

2714c13

fixup

99b91a5

fixup

a867f00

Merge remote-tracking branch 'upstream/main' into expand-on-scalars

9e13924

fixup

f197672

MarcoGorelli commented Nov 9, 2023

View reviewed changes

MarcoGorelli added 4 commits November 14, 2023 19:06

Merge remote-tracking branch 'upstream/main' into expand-on-scalars

a417b1c

replace Scalar|NullType with Scalar

409d8f3

type null as Scalar

0db9871

add example of working with scalars

6520ac4

kkraus14 reviewed Nov 14, 2023

View reviewed changes

kkraus14 mentioned this pull request Nov 14, 2023

add column.shift #317

Merged

use AnyScalar;

46bc08c

MarcoGorelli marked this pull request as draft November 15, 2023 11:53

MarcoGorelli added 3 commits November 15, 2023 12:32

add Scalar.dtype and Scalar.persist

b879f31

Merge remote-tracking branch 'upstream/main' into expand-on-scalars

456c152

update shift arg

b8011c7

MarcoGorelli marked this pull request as ready for review November 15, 2023 13:51

MarcoGorelli added 6 commits November 15, 2023 14:33

use BoolScalar

97d8f9a

use float scalar in some parts

3b7bcb6

use float scalar in some parts

d598a8d

string scalar for rename

a12585b

intscalar for shift

35cd4ed

numeric scalar for correction

fade164

MarcoGorelli requested a review from kkraus14 November 15, 2023 18:32

MarcoGorelli added 6 commits November 15, 2023 20:03

simplify

29ceed2

update python builtin types desc

bee402f

Merge remote-tracking branch 'upstream/main' into expand-on-scalars

d1f4daf

fixup

15090ac

enable extra ruff rule, note AnyScalar

24d2ad8

remove some unnecessary nitpick ignores

8360d96

kkraus14 reviewed Nov 15, 2023

View reviewed changes

MarcoGorelli mentioned this pull request Nov 16, 2023

Rename entrypoint to __consortium_api__? #323

Closed

MarcoGorelli requested a review from kkraus14 November 16, 2023 13:50

kkraus14 approved these changes Nov 16, 2023

View reviewed changes

MarcoGorelli added 2 commits November 17, 2023 12:39

return Self from Scalar.persist, add column.persist

f69679a

fixup

216b5e6

MarcoGorelli merged commit 1f81476 into data-apis:main Nov 17, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add details to expectations for scalars #308

Add details to expectations for scalars #308

MarcoGorelli commented Oct 31, 2023

kkraus14 commented Oct 31, 2023 •

edited

Loading

MarcoGorelli commented Nov 1, 2023

kkraus14 commented Nov 1, 2023

MarcoGorelli commented Nov 1, 2023

MarcoGorelli commented Nov 9, 2023

MarcoGorelli Nov 9, 2023

MarcoGorelli commented Nov 10, 2023 •

edited

Loading

MarcoGorelli commented Nov 14, 2023

kkraus14 Nov 10, 2023

MarcoGorelli Nov 14, 2023 •

edited

Loading

kkraus14 Nov 10, 2023

MarcoGorelli Nov 14, 2023

MarcoGorelli Nov 14, 2023

kkraus14 Nov 14, 2023

kkraus14 Nov 10, 2023

MarcoGorelli Nov 14, 2023

MarcoGorelli Nov 14, 2023

kkraus14 Nov 14, 2023

MarcoGorelli Nov 14, 2023

MarcoGorelli Nov 14, 2023

kkraus14 Nov 14, 2023

MarcoGorelli Nov 14, 2023

kkraus14 Nov 15, 2023

MarcoGorelli commented Nov 15, 2023 •

edited

Loading

kkraus14 Nov 15, 2023

MarcoGorelli Nov 16, 2023

kkraus14 Nov 15, 2023

kkraus14 Nov 15, 2023

MarcoGorelli Nov 15, 2023

MarcoGorelli commented Nov 16, 2023

MarcoGorelli commented Nov 16, 2023

		@@ -169,15 +167,31 @@ def __column_consortium_standard__(
		...


		PythonScalar = Union[str, int, float, bool]

Add details to expectations for scalars #308

Add details to expectations for scalars #308

Conversation

MarcoGorelli commented Oct 31, 2023

kkraus14 commented Oct 31, 2023 • edited Loading

MarcoGorelli commented Nov 1, 2023

kkraus14 commented Nov 1, 2023

MarcoGorelli commented Nov 1, 2023

MarcoGorelli commented Nov 9, 2023

Choose a reason for hiding this comment

MarcoGorelli commented Nov 10, 2023 • edited Loading

MarcoGorelli commented Nov 14, 2023

Choose a reason for hiding this comment

MarcoGorelli Nov 14, 2023 • 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

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

MarcoGorelli commented Nov 15, 2023 • 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

MarcoGorelli commented Nov 16, 2023

MarcoGorelli commented Nov 16, 2023

kkraus14 commented Oct 31, 2023 •

edited

Loading

MarcoGorelli commented Nov 10, 2023 •

edited

Loading

MarcoGorelli Nov 14, 2023 •

edited

Loading

MarcoGorelli commented Nov 15, 2023 •

edited

Loading