Typehints and keyword-only args for binning #2012

jl-wynen · 2021-06-30T14:46:18Z

No description provided.

SimonHeybrock · 2021-07-01T04:37:06Z

docs/about/whats-new.ipynb

@@ -774,8 +774,8 @@
    "        'y':sc.Variable(dims=['time'], unit='m', values=np.random.rand(N)),\n",
    "        'time':sc.Variable(dims=['time'], values=(10000*np.random.rand(N)).astype('datetime64[s]')),\n",
    "    })\n",
-    "binned = sc.bin(data, edges=[sc.linspace(dim='x', unit='m', start=0.0, stop=1.0, num=5),\n",
-    "                             sc.linspace(dim='y', unit='m', start=0.0, stop=1.0, num=5)])\n",
+    "binned = sc.bin(data, bins=[sc.linspace(dim='x', unit='m', start=0.0, stop=1.0, num=5),\n",


Why the rename? That should be a separate discussion, see #1618.

To make it consistent with histogram.
I thought while I am breaking the API I might as well get this one out of the way. Would you rather postpone it?

My point is: we should have a discussion first before making API changes.

What if we actually prefer to call it edges every rather than bins? We don't want to break things twice.

SimonHeybrock · 2021-07-01T04:38:42Z

docs/user-guide/binned-data/computation.ipynb

@@ -297,7 +297,7 @@
   "outputs": [],
   "source": [
    "sc.show(a['x',2].value)\n",
-    "a.bins.concatenate(a, out=a)\n",
+    "a.bins.concatenate(other=a, out=a)\n",


I think in 95%+ uses we only have other, should we support that without keyword?

I am generally more in favour of not requiring keywords in most places, so yes?

I think requiring it for all but the first (non-optional) arg is good, e.g., out in this case should always be keyword only?

SimonHeybrock · 2021-07-01T04:40:27Z

python/src/scipp/_bins.py

        """Sum of each bin.

        :return: The sum of each of the input bins.
        :seealso: :py:func:`scipp.sum` for summing non-bin data
        """
        return _call_cpp_func(_cpp.buckets.sum, self._obj)

-    def size(self):
+    def size(self) -> _cpp.DataArray:


Data array or variable?

SimonHeybrock · 2021-07-01T04:41:03Z

python/src/scipp/_bins.py

        """Set data of the bins"""
        _cpp._bins_view(self._data()).data = data

    @property
-    def constituents(self):
+    def constituents(self) -> Dict[str, Union[_cpp.Variable, _cpp.DataArray]]:


also str in the union.

SimonHeybrock · 2021-07-01T04:42:06Z

python/src/scipp/_bins.py

+                    other: Optional[Union[_cpp.Variable,
+                                          _cpp.DataArray]] = None,
+                    dim: Optional[str] = None,
+                    out: Optional[_cpp.DataArray] = None) -> _cpp.DataArray:


If self and other are variable, so is the return type?

SimonHeybrock · 2021-07-01T04:44:32Z

python/src/scipp/typing.py

@@ -49,3 +49,6 @@ def has_numeric_type(obj: _std_typing.Any) -> bool:

 #: Any object that behaves like a scipp.Dataset in most operations.
 DatasetLike = _std_typing.Union[DataArrayLike, sc.Dataset]


I still feel this is very misleading, since most users typically use data arrays, which do not behave like datasets for a number of aspects.

Unify DatasetLike and DataArrayLike into LabelledArrayLike?

I'm not sure LabeledArray is the best option here, as to me it would suggest that there is only one array, so it would be for a DataArray. It feels like it doesn't fit Dataset so well...
But I don't have any good alternatives to suggest :-(

SimonHeybrock · 2021-07-01T04:47:17Z

python/tests/dynamic_binding_test.py

+        da.groupby(group='x').sum('x'),
+        sc.groupby(da, group='x').sum('x'))


Interesting: What is the best style for functions that typically take only a single mandatory argument? Is enforcing keyword args really the right solution there? I agree that it is the correct way in the second line (since there is more than one argument), but what about the first line? The keyword arg is just repeating the function name. da.groupby('x') should be obvious?

da.groupby('x') should be obvious?

Not sure I agree. Since groupby has this additional function of binning, it is not super clear which argument is which unless you name them.

But the main problem is, can the generic binding function decide which arguments should be keyword only and which positional? It would be hard to take subtleties like arguments with the same type into account. But of course we can bind groupby manually.

But isn't the first (group) arg always required, even if bins are given?

Yes. But that was never part of the discussion on requiring keywords. E.g. array requires dims and values but they are still keyword-only.

You are right that it is unclear. Personally I feel that any function that has exactly 1 required argument is ok without keywords. For example:

sc.scalar works without value=.

sc.sum(da) works

da.groupby('x') feels natural

Isn't one of the main reasons for keyword args to avoid mixing up arg order? In none of the above cases there is any risk for that, I believe?

It gets more blurry for things such as sc.sum(da, 'x'), which has 2 args. I still feel that is fine, but maybe that is just because I am used to it? Maybe because da.sum('x') does the same and is also clear?

How about this guideline (exceptions may be considered individually):

Functions with a single arg are ok (sc.sin(da)).

Functions with two args are ok provided that one of these is fulfilled:

Both args are scipp objects (sc.less(da1, da2)). Exception: sc.atan2 which has a potentially confusing arg order

Second args is a dim (sc.sum(var, 'x')). Maybe an alternative formulation could be: If the func is also bounds as a method with a single arg, it is ok (because bascically it is a variant of the topmost bullet) (var.sum('x') is ok, therefore sc.sum(var, 'x') is ok).

Any further optional args must be keyword args.

Any functions that are not in the categories above should generally require keyword args.

I mostly agree. But what about to_unit?
Maybe we can change ii. to functions where the first arg is a scipp object. Those can be seen analogous to methods in a Universal Function Call Syntax kind of way. In this case it would always be ok to have the first and potential second arg positional. All additional args would be keyword-only.

Suggestion above sounds reasonable to me. Seems to balance enforcing keyword arguments where we really should avoid ambiguity and unnecessary typing where there is none. The atan2 example is maybe one to bear in mind as we implement new things, I can't think of any strong similar examples at present (maybe cross product if we had that), but there may be other exceptions like this.

I also agree that you should allow non-keyword arguments where you only have one argument, or when using free functions like sc.sum().
I think users would be rapidly annoyed if they have to use kwargs everywhere.

SimonHeybrock · 2021-07-02T05:45:48Z

docs/conf.py

-    'DataArrayLike': 'DataArrayLike',
-    'DatasetLike': 'DatasetLike',
+    'LabeledArray': 'LabeledArray',
+    'MetaDataMap': 'MetaDataMap',


The latter refers to Coord or Masks? Would Dict instead of Map be more understandable to a Python user?

Maybe. The corresponding class in typing is MutableMapping, see definition of MetaDataMap.

SimonHeybrock · 2021-07-02T05:50:39Z

python/src/scipp/_comparison.py



-def less(x: DataArrayLike, y: DataArrayLike) -> DataArrayLike:
+def less(x: LabeledArray, y: LabeledArray) -> LabeledArray:


Are we doing ourselves a favor by introducing a 4th term, LabeledArray? It feels like a synonym for what a DataArray is, but we also mean variables and datasets? What do we gain? Isn't this adding more confusion than necessary? Or is this never displayed to the user, and instead lists the union of variable, data array, and dataset?

With the aliases set up in conf.py, 'LabeledArray' is shown in the docs. I could take that out, then Sphinx expands it and shows the Union. But it would still show up in the source and help messages.

SimonHeybrock · 2021-07-02T05:51:30Z

python/src/scipp/_math.py

@@ -70,8 +69,7 @@ def reciprocal(x: DataArrayLike,
    return _call_cpp_func(_cpp.reciprocal, x, out=out)


-def sqrt(x: DataArrayLike,
-         out: Optional[DataArrayLike] = None) -> DataArrayLike:
+def sqrt(x: LabeledArray, out: Optional[LabeledArray] = None) -> LabeledArray:


Should out args always require a keyword arg? I think so.

SimonHeybrock · 2021-07-02T05:54:28Z

python/src/scipp/typing.py

-DataArrayLike = _std_typing.Union[sc.Variable, sc.DataArray]
+#: Any object that behaves like a scipp.Variable,
+#:  that is an array with labeled dimensions.
+LabeledArray = _std_typing.Union[sc.Variable, sc.DataArray, sc.Dataset]


Oh, now I understand, "labeled" refers to the dims, not the coords (which would be labeling the elements). Then the name does make sense, but is still ambiguous. :(

Suggestions are welcome!

We could just use VariableLike?

No idea, I am giving up for now, I think :(

jl-wynen added 11 commits June 30, 2021 16:44

Make arguments of bins keyword only

65ac66a

Add type hints to binning functions

2fecc68

More explicit binning docstrings

8db9a95

Remove GroupBy methods from free function docs

006546f

Make args of bin and histogram kwarg only

ad9094c

Propagate keyword defaults to bound methods

1d6f138

Propagate annotations to bound methods

5e82943

Make groupby args kwonly

0697843

Add typehints to Bins

825387e

Make Bins.concatenate args keyword only

8d79271

Update docs

0a384a0

jl-wynen requested a review from SimonHeybrock June 30, 2021 14:46

jl-wynen mentioned this pull request Jun 30, 2021

Improve Python bindings #1302

Closed

8 tasks

Update release notes

f64d28d

SimonHeybrock reviewed Jul 1, 2021

View reviewed changes

jl-wynen added 8 commits July 1, 2021 13:39

Fix typehints in Bins methods

e65c11d

Revert argument name bins->edges

2e46163

Make other arg of concatenate no keyword only

2371d38

Add MetaDataMap to API ref

156c0c0

Combine DataArrayLike,DatasetLike into LabeledArray

db7d27e

Merge branch 'main' into typehints

f45fd9d

Make group arg positional

3a5c0c5

yapf

c4b9663

SimonHeybrock reviewed Jul 2, 2021

View reviewed changes

jl-wynen added 3 commits July 2, 2021 12:55

Make out args keyword only

a5c0a2f

Rename LabeledArray -> VariableLike

ecb0ddb

Merge branch 'main' into typehints

0b121cb

SimonHeybrock approved these changes Jul 8, 2021

View reviewed changes

SimonHeybrock merged commit 87de0c3 into main Jul 8, 2021

SimonHeybrock deleted the typehints branch July 8, 2021 04:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typehints and keyword-only args for binning #2012

Typehints and keyword-only args for binning #2012

jl-wynen commented Jun 30, 2021

SimonHeybrock Jul 1, 2021

jl-wynen Jul 1, 2021

SimonHeybrock Jul 1, 2021

jl-wynen Jul 1, 2021

SimonHeybrock Jul 1, 2021

jl-wynen Jul 1, 2021

SimonHeybrock Jul 1, 2021

SimonHeybrock Jul 1, 2021

SimonHeybrock Jul 1, 2021

SimonHeybrock Jul 1, 2021

SimonHeybrock Jul 1, 2021

jl-wynen Jul 1, 2021

SimonHeybrock Jul 1, 2021

nvaytet Jul 1, 2021 •

edited

Loading

SimonHeybrock Jul 1, 2021

jl-wynen Jul 1, 2021

SimonHeybrock Jul 1, 2021

jl-wynen Jul 1, 2021

SimonHeybrock Jul 1, 2021

SimonHeybrock Jul 1, 2021 •

edited

Loading

jl-wynen Jul 1, 2021

OwenArnold Jul 1, 2021

nvaytet Jul 1, 2021

SimonHeybrock Jul 2, 2021 •

edited

Loading

jl-wynen Jul 2, 2021

SimonHeybrock Jul 2, 2021

jl-wynen Jul 2, 2021

SimonHeybrock Jul 2, 2021

jl-wynen Jul 2, 2021

SimonHeybrock Jul 2, 2021

jl-wynen Jul 2, 2021

SimonHeybrock Jul 2, 2021

		@@ -49,3 +49,6 @@ def has_numeric_type(obj: _std_typing.Any) -> bool:

		#: Any object that behaves like a scipp.Dataset in most operations.
		DatasetLike = _std_typing.Union[DataArrayLike, sc.Dataset]

		da.groupby(group='x').sum('x'),
		sc.groupby(da, group='x').sum('x'))



		def less(x: DataArrayLike, y: DataArrayLike) -> DataArrayLike:
		def less(x: LabeledArray, y: LabeledArray) -> LabeledArray:

Typehints and keyword-only args for binning #2012

Typehints and keyword-only args for binning #2012

Conversation

jl-wynen commented Jun 30, 2021

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

nvaytet Jul 1, 2021 • 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

SimonHeybrock Jul 1, 2021 • 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

SimonHeybrock Jul 2, 2021 • 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

nvaytet Jul 1, 2021 •

edited

Loading

SimonHeybrock Jul 1, 2021 •

edited

Loading

SimonHeybrock Jul 2, 2021 •

edited

Loading