Add assign_(coord/mask/attr) methods of data_array #3110

YooSunYoung · 2023-04-05T12:42:25Z

TODO: python binding is not implemented yet

I used the name assign since I thought it should also be able to update the existing one.
Like this: https://docs.xarray.dev/en/stable/generated/xarray.DataArray.assign_coords.html

In the xarray, the syntax is like below

values = [...]
coord_name = "coord0"
dimension_name = "dim0"
assign_coords(coord_name=(dimension_name, values))

But since we are using the Variable which already has dimension name, it should be like below...?

coord_name = "coord0"
coord0 = sc.Variable(dims=["dim0"], values=[...])
da.assign_coords(coord_name=coord0)

…luding basic tests

YooSunYoung · 2023-04-05T12:43:55Z

I have a question!

Should we check if the given coords/masks/attrs has overlapping keys?

SimonHeybrock · 2023-04-11T07:06:41Z

TODO: python binding is not implemented yet

Any reason why we don't implement the entire thing in Python?

SimonHeybrock · 2023-04-11T07:09:11Z

I have a question!

Should we check if the given coords/masks/attrs has overlapping keys?

You mean if we are replacing existing coords? I think I would not check. After all, __setitem__ does not check either?

YooSunYoung · 2023-04-11T07:29:33Z

TODO: python binding is not implemented yet

Any reason why we don't implement the entire thing in Python?

No... drop_* were in cpp so I just kept there together. We can write all of them in python, sure.

SimonHeybrock · 2023-04-11T07:38:15Z

TODO: python binding is not implemented yet

Any reason why we don't implement the entire thing in Python?

No... drop_* were in cpp so I just kept there together. We can write all of them in python, sure.

👍

YooSunYoung · 2023-04-11T07:41:23Z

I have a question!
Should we check if the given coords/masks/attrs has overlapping keys?

You mean if we are replacing existing coords? I think I would not check. After all, __setitem__ does not check either?

No, I meant when you want to assign multiple of them, for example

assign_coords({'a': coord0, 'b':coord1, 'a': coord2})

SimonHeybrock · 2023-04-11T08:03:21Z

I have a question!
Should we check if the given coords/masks/attrs has overlapping keys?

You mean if we are replacing existing coords? I think I would not check. After all, __setitem__ does not check either?

No, I meant when you want to assign multiple of them, for example
assign_coords({'a': coord0, 'b':coord1, 'a': coord2})

This will never reach assign_coords anyway, because Python first creates a dict?

This reverts commit 2f9a4e9.

…rary including basic tests" This reverts commit 7dbe6ca.

YooSunYoung · 2023-04-11T15:37:17Z

I have a question!
Should we check if the given coords/masks/attrs has overlapping keys?

You mean if we are replacing existing coords? I think I would not check. After all, __setitem__ does not check either?

No, I meant when you want to assign multiple of them, for example
assign_coords({'a': coord0, 'b':coord1, 'a': coord2})
This will never reach assign_coords anyway, because Python first creates a dict?

In this case below it will survive but the keyword arguments will be used. I guess that's fine...? Or should we allow only one of them...?

assign_coords({'a': coord0}, a=coord0)

SimonHeybrock

Comments below. Everything I said for the coords case applies to the others as well.

src/scipp/core/assignments.py

SimonHeybrock · 2023-04-12T06:42:54Z

src/scipp/core/assignments.py

+
+
+def assign_coords(
+    self, coords: Optional[Dict] = None, **coords_kwargs


coords should be position-only, such that it does not prevent the use of a keyword arg of that name.

SimonHeybrock · 2023-04-12T06:44:00Z

src/scipp/core/assignments.py

+        collected_coords = coords_kwargs
+
+    for coord_key, coord in collected_coords.items():
+        self.coords[coord_key] = coord


We should not change the input. Must make a shallow copy first (self = self.copy(deep=False))

SimonHeybrock · 2023-04-12T06:45:16Z

tests/data_array_test.py

+    da = sc.DataArray(data)
+    coord0 = sc.linspace('x', start=0.2, stop=1.61, num=4)
+    coord1 = sc.linspace('y', start=1, stop=4, num=3)
+    da.assign_coords({'coord0': coord0, 'coord1': coord1})


da should be kept unchanged. Make sure to also a a test for that.

Co-authored-by: Simon Heybrock <12912489+SimonHeybrock@users.noreply.github.com>

…opy before modification

SimonHeybrock · 2023-04-12T08:48:52Z

src/scipp/core/assignments.py

+from ..typing import DataArray, Dataset
+
+
+def assign_coords(self, coords: Dict) -> Union[DataArray, Dataset]:


Why did you remove the keyword-arg syntax?

coords should be position-only, such that it does not prevent the use of a keyword arg of that name.

Sorry I misunderstood this comment. Then... what do you mean by position-only here ...?

I meant the dict should be position-only, but keep the keyword args.

def assign_coords(self, coords: Dict[str, Variable], /, **kwargs):

or something like that

Ah, I see. My question was more like, should we allow only either of them

More specifically, will this be allowed?

da.assign_coords({'coord0': coord0_a}, coord0=coord0_b)

Currently, the keyword argument will overwrite the positional argument in this case.

…s with better overlapping key-words handling

YooSunYoung · 2023-04-12T12:12:25Z

rename_dims allows using both positional arguments and keyword arguments,
but check if there is any overlapping names as you said,
so I did the similar thing for assign_*.

SimonHeybrock · 2023-04-12T12:13:42Z

src/scipp/core/assignments.py

+    coords_posarg = {} if coords is None else coords
+    collected_coords = {**coords_posarg, **coords_kwargs}
+
+    if len(collected_coords) != len(coords_posarg) + len(coords_kwargs):
+        overlapped = set(coords).intersection(coords_kwargs)
+        raise ValueError(
+            'The names of coords passed in the dict '
+            'and as keyword arguments must be distinct.'
+            f'Following names were used in both places: {overlapped}.'
+        )


Can all this code duplication be avoided?

I can write a general function that combines positional dictionary argument and keyword arguments I guess...?

Looks like it exists already:

scipp/src/scipp/core/dimensions.py

Lines 12 to 21 in 62778d2

def _combine_dims(

dims_dict: Optional[Dict[str, str]], names: Dict[str, str]

) -> Dict[str, str]:

dims_dict = {} if dims_dict is None else dims_dict

if set(dims_dict).intersection(names):

raise ValueError(

'The names passed in the dict and as keyword arguments must be distinct.'

f'Got {dims_dict} and {names}'

)

return {**dims_dict, **names}

Yes, that what I referenced from.
It prints all dictionaries here but since coords/attrs/masks are Variable,
but I didn't want to print them all so I just changed the message a little.

… assign_* methods

SimonHeybrock · 2023-04-12T12:33:51Z

src/scipp/core/assignments.py

@@ -8,6 +8,21 @@
 from ..typing import DataArray, Dataset, Variable


+def _combine_args(arg0: Optional[Dict[str, Variable]] = None, /, **kwargs):


Still duplicate with

scipp/src/scipp/core/dimensions.py

Lines 12 to 21 in 62778d2

def _combine_dims(

dims_dict: Optional[Dict[str, str]], names: Dict[str, str]

) -> Dict[str, str]:

dims_dict = {} if dims_dict is None else dims_dict

if set(dims_dict).intersection(names):

raise ValueError(

'The names passed in the dict and as keyword arguments must be distinct.'

f'Got {dims_dict} and {names}'

)

return {**dims_dict, **names}

. You can keep the error and not worry about it (since it should be extremely rare), or adjust it there to only print the keys?

…ave even less duplicated lines

SimonHeybrock · 2023-04-13T04:54:44Z

src/scipp/core/argument_handlers.py

+from typing import Dict, Optional
+
+
+def combine_dict_args(arg: Optional[Dict], /, **kwargs):


type hints missing now

Add assign_(coord/mask/attr) methods of data_array in cpp library inc…

7dbe6ca

…luding basic tests

YooSunYoung requested review from jl-wynen and SimonHeybrock April 5, 2023 12:51

Organize functions by names

2f9a4e9

Merge branch 'main' into feature-assign

9045444

YooSunYoung added 4 commits April 11, 2023 17:07

Revert "Organize functions by names"

3058da3

This reverts commit 2f9a4e9.

Revert "Add assign_(coord/mask/attr) methods of data_array in cpp lib…

ba038ef

…rary including basic tests" This reverts commit 7dbe6ca.

Add assign_coords/masks/attrs to DataArray and assign_coords to Dataset

3604e4d

Add release note

26d1fa8

YooSunYoung marked this pull request as ready for review April 11, 2023 15:38

SimonHeybrock requested changes Apr 12, 2023

View reviewed changes

YooSunYoung and others added 3 commits April 12, 2023 09:03

Update src/scipp/core/assignments.py

39d4aae

Co-authored-by: Simon Heybrock <12912489+SimonHeybrock@users.noreply.github.com>

Update src/scipp/core/assignments.py

0a7e266

Co-authored-by: Simon Heybrock <12912489+SimonHeybrock@users.noreply.github.com>

Remove keyword arguments from assign_* methods and create a shallow c…

c4776e9

…opy before modification

SimonHeybrock reviewed Apr 12, 2023

View reviewed changes

YooSunYoung added 3 commits April 12, 2023 13:18

Add keyword arguments in assign_* methods

4a4c833

Merge branch 'main' into feature-assign

1623d04

More specific description of positional argument and keyword argument…

512a3e0

…s with better overlapping key-words handling

SimonHeybrock reviewed Apr 12, 2023

View reviewed changes

YooSunYoung added 2 commits April 12, 2023 14:14

Rename the unit-test

329f9dc

Add arguments combining helper function to remove duplicated lines in…

7288946

… assign_* methods

Adjust error message

19626ab

YooSunYoung requested a review from SimonHeybrock April 12, 2023 12:30

SimonHeybrock reviewed Apr 12, 2023

View reviewed changes

YooSunYoung added 2 commits April 12, 2023 17:37

Dictionary type argument merging function to minimize duplicated lines

5362a29

Add more general _assign method to use in the assign_* functions to h…

7fbd30a

…ave even less duplicated lines

SimonHeybrock reviewed Apr 13, 2023

View reviewed changes

Add type hints in dict-like arguments merging helper function

133fc3e

SimonHeybrock approved these changes Apr 13, 2023

View reviewed changes

YooSunYoung added 2 commits April 13, 2023 09:11

Argument combining without unpacking keyword arguments

3e193a1

Merge branch 'main' into feature-assign

223b9f5

YooSunYoung merged commit 362e921 into scipp:main Apr 13, 2023
4 checks passed

YooSunYoung deleted the feature-assign branch April 13, 2023 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add assign_(coord/mask/attr) methods of data_array #3110

Add assign_(coord/mask/attr) methods of data_array #3110

YooSunYoung commented Apr 5, 2023 •

edited

YooSunYoung commented Apr 5, 2023

SimonHeybrock commented Apr 11, 2023

SimonHeybrock commented Apr 11, 2023

YooSunYoung commented Apr 11, 2023

SimonHeybrock commented Apr 11, 2023

YooSunYoung commented Apr 11, 2023

SimonHeybrock commented Apr 11, 2023

YooSunYoung commented Apr 11, 2023

SimonHeybrock left a comment

SimonHeybrock Apr 12, 2023

SimonHeybrock Apr 12, 2023

SimonHeybrock Apr 12, 2023

SimonHeybrock Apr 12, 2023

YooSunYoung Apr 12, 2023

SimonHeybrock Apr 12, 2023

SimonHeybrock Apr 12, 2023 •

edited

YooSunYoung Apr 12, 2023

YooSunYoung commented Apr 12, 2023

SimonHeybrock Apr 12, 2023

YooSunYoung Apr 12, 2023

SimonHeybrock Apr 12, 2023

YooSunYoung Apr 12, 2023

SimonHeybrock Apr 12, 2023

SimonHeybrock Apr 13, 2023



		def assign_coords(
		self, coords: Optional[Dict] = None, **coords_kwargs

		from ..typing import DataArray, Dataset


		def assign_coords(self, coords: Dict) -> Union[DataArray, Dataset]:

	def _combine_dims(
	dims_dict: Optional[Dict[str, str]], names: Dict[str, str]
	) -> Dict[str, str]:
	dims_dict = {} if dims_dict is None else dims_dict
	if set(dims_dict).intersection(names):
	raise ValueError(
	'The names passed in the dict and as keyword arguments must be distinct.'
	f'Got {dims_dict} and {names}'
	)
	return {dims_dict, names}

		@@ -8,6 +8,21 @@
		from ..typing import DataArray, Dataset, Variable


		def _combine_args(arg0: Optional[Dict[str, Variable]] = None, /, **kwargs):

		from typing import Dict, Optional


		def combine_dict_args(arg: Optional[Dict], /, **kwargs):

Add assign_(coord/mask/attr) methods of data_array #3110

Add assign_(coord/mask/attr) methods of data_array #3110

Conversation

YooSunYoung commented Apr 5, 2023 • edited

YooSunYoung commented Apr 5, 2023

SimonHeybrock commented Apr 11, 2023

SimonHeybrock commented Apr 11, 2023

YooSunYoung commented Apr 11, 2023

SimonHeybrock commented Apr 11, 2023

YooSunYoung commented Apr 11, 2023

SimonHeybrock commented Apr 11, 2023

YooSunYoung commented Apr 11, 2023

SimonHeybrock left a 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

SimonHeybrock Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YooSunYoung commented Apr 12, 2023

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

YooSunYoung commented Apr 5, 2023 •

edited

SimonHeybrock Apr 12, 2023 •

edited