fuse, split, MultiStreamScoreMean, and merge all #767

dafnapension · 2024-04-13T19:12:56Z

of each instance
per issue #764

elronbandel · 2024-04-14T11:05:14Z

src/unitxt/fusion.py

    """

-    origins: List[SourceOperator]
+    origins: Dict[str, SourceOperator]


Suggested change

origins: Dict[str, SourceOperator]

origins: Union[List[SourceOperator], Dict[str, SourceOperator]]

I think we should allow also anonymus sources merging which are merged without given name per source

elronbandel · 2024-04-14T11:11:30Z

src/unitxt/fusion.py

+                            if "past_groups" in instance:
+                                instance["past_groups"] = (
+                                    instance["group"] + "/" + instance["past_groups"]
+                                )
+                            else:
+                                instance["past_groups"] = instance["group"]
+                        instance["group"] = origin_name


Suggested change

if "past_groups" in instance:

instance["past_groups"] = (

instance["group"] + "/" + instance["past_groups"]

)

else:

instance["past_groups"] = instance["group"]

instance["group"] = origin_name

group = origin_name + "/" + instance["group"]

else:

group = origin_name

instance["group"] = group

Why not like this? more simple and dont require additional variable

I thought that code that reads the "group" (e.g, the splitter) will not need to check if any past is glued to this group, and if glued -- peel it off. Only if the consuming code is explicitly interested in the past, it will look into "past_groups", and do whatever it wants.

Changed per your request, @elronbandel. We will know what to do if the past haunts us..

elronbandel · 2024-04-14T11:12:15Z

src/unitxt/fusion.py

+                    if "group" in instance:
+                        if "past_groups" in instance:
+                            instance["past_groups"] = (
+                                instance["group"] + "/" + instance["past_groups"]
+                            )
+                        else:
+                            instance["past_groups"] = instance["group"]
+                    instance["group"] = origin_name


Same here dont think past_groups are necessary

Yes, same thing: trying to work here, in order to save work from the consure (e.g. Splitter)

Changed here too

elronbandel

I added some comments, in general:

I think we should have only one field "group" containing all the grouping information.
I think we should test also hirarchial group naming works within the unittests
I think we should allow also Fusion of streams without names
We should test the tests actually reutrn score per group with the existing mecahinsm (i linked the exact line in the metric pipeline in the related issue)

dafnapension · 2024-04-14T12:10:26Z

I added some comments, in general:

I think we should have only one field "group" containing all the grouping information.

I think we should test also hirarchial group naming works withing the unittests

I think we should allow also Fusion of streams without names

We should test the tests actually reutrn score per group with the existing mecahinsm (i linked the exact line in the metric pipeline in the related issue)

OK. I just thought to save work from the consuming code, but accept your judgement, of course.

Exactly this line that you linked to in the issue, reads:
SplitByValue(["group"])

As it is, it can not work when group carries the whole history, unless you would like indeed to split them all by the whole history, and not just by the most recent fuse.

codecov · 2024-05-05T15:35:43Z

Codecov Report

Attention: Patch coverage is 96.19048% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 92.14%. Comparing base (5783d76) to head (210b4d5).
Report is 42 commits behind head on main.

❗ Current head 210b4d5 differs from pull request most recent head e829415. Consider uploading reports for the commit e829415 to get more accurate results

Files	Patch %	Lines
src/unitxt/fusion.py	93.02%	3 Missing ⚠️
src/unitxt/operators.py	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #767      +/-   ##
==========================================
+ Coverage   91.48%   92.14%   +0.65%     
==========================================
  Files         100      103       +3     
  Lines       10169    10614     +445     
==========================================
+ Hits         9303     9780     +477     
+ Misses        866      834      -32

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

elronbandel · 2024-05-08T09:55:47Z

src/unitxt/operators.py

@@ -1560,6 +1560,48 @@ def process(self, multi_stream: MultiStream) -> MultiStream:
        return MultiStream(result)


+class SplitByGroup(MultiStreamOperator):


Suggested change

class SplitByGroup(MultiStreamOperator):

class SplitByNestedGroup(MultiStreamOperator):

field: str = "group"

dafnapension · 2024-05-11T20:48:26Z

Hi @elronbandel , I hope this is more in the direction you had in mind.

elronbandel · 2024-05-12T06:55:41Z

src/unitxt/fusion.py

-        splits: List of splits to include. If None, all splits are included.
+        origins: Dict of named SourceOperator objects (each to yield a MultiStream), or a list thereof
+        splits: List of splits (stream_names) to include, over all input multistreams. If None, all splits are included.
+        max_instances_per_origin_split: Number of instances to take from each input split of each input multistream.


Great catch! the old name was disambgious!

Signed-off-by: dafnapension <dafnashein@yahoo.com>

… the operation done Signed-off-by: dafnapension <dafnashein@yahoo.com>

Signed-off-by: dafnapension <dafnashein@yahoo.com>

…of just one stream - it returns it as is. No need to generate a nested structure for just one leaf. Signed-off-by: dafnapension <dafnashein@yahoo.com>

elronbandel reviewed Apr 14, 2024

View reviewed changes

elronbandel requested changes Apr 14, 2024

View reviewed changes

dafnapension force-pushed the fuse_named_resources branch from a1976e7 to 03c2897 Compare April 15, 2024 17:46

dafnapension changed the title ~~name the sources for fusion and document that name in field 'group'~~ name the sources for fusion and document that name in field 'group' , yet allow anonymous sources too Apr 15, 2024

dafnapension force-pushed the fuse_named_resources branch from 809f987 to a9dc1ff Compare April 17, 2024 13:12

dafnapension force-pushed the fuse_named_resources branch 5 times, most recently from e479174 to 210b4d5 Compare May 5, 2024 15:26

dafnapension force-pushed the fuse_named_resources branch 2 times, most recently from b2203d4 to 7210ed8 Compare May 8, 2024 08:03

elronbandel reviewed May 8, 2024

View reviewed changes

dafnapension force-pushed the fuse_named_resources branch 2 times, most recently from 7da0bfc to fceaeca Compare May 10, 2024 19:13

dafnapension changed the title ~~name the sources for fusion and document that name in field 'group' , yet allow anonymous sources too~~ fuse, split, MultiStreamScoreMean, and merge all May 10, 2024

dafnapension force-pushed the fuse_named_resources branch from 360bd25 to 60fc7b0 Compare May 11, 2024 05:52

elronbandel reviewed May 12, 2024

View reviewed changes

elronbandel approved these changes May 12, 2024

View reviewed changes

dafnapension added 6 commits May 12, 2024 15:24

name fusing resources and allow annonymous fusing too

696487b

Signed-off-by: dafnapension <dafnashein@yahoo.com>

allow anonymous too

3d265ab

Signed-off-by: dafnapension <dafnashein@yahoo.com>

test fused and split through metric

d9e00b4

Signed-off-by: dafnapension <dafnashein@yahoo.com>

fuse, split, MultiStreamScoreMean, and merge all

91f91ae

Signed-off-by: dafnapension <dafnashein@yahoo.com>

never peek twice

23187ce

Signed-off-by: dafnapension <dafnashein@yahoo.com>

renamed operator SplitByGroup -> SplitByNestedGroup to better reflect…

0a93654

… the operation done Signed-off-by: dafnapension <dafnashein@yahoo.com>

dafnapension added 2 commits May 12, 2024 15:24

coherent structure for global score of a multistream

7bf2ba9

Signed-off-by: dafnapension <dafnashein@yahoo.com>

when MultiStreamScoreMean receives as input, multi-stream consisting …

e829415

…of just one stream - it returns it as is. No need to generate a nested structure for just one leaf. Signed-off-by: dafnapension <dafnashein@yahoo.com>

dafnapension force-pushed the fuse_named_resources branch from 9ae5ee7 to e829415 Compare May 12, 2024 12:24

dafnapension merged commit 1b1f6d4 into main May 12, 2024
7 checks passed

dafnapension deleted the fuse_named_resources branch May 12, 2024 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fuse, split, MultiStreamScoreMean, and merge all #767

fuse, split, MultiStreamScoreMean, and merge all #767

dafnapension commented Apr 13, 2024 •

edited

elronbandel Apr 14, 2024 •

edited

dafnapension Apr 14, 2024 •

edited

elronbandel Apr 14, 2024

dafnapension Apr 14, 2024 •

edited

dafnapension Apr 15, 2024

elronbandel Apr 14, 2024

dafnapension Apr 14, 2024

dafnapension Apr 15, 2024

elronbandel left a comment •

edited

dafnapension commented Apr 14, 2024 •

edited

codecov bot commented May 5, 2024 •

edited

elronbandel May 8, 2024

dafnapension commented May 11, 2024

elronbandel May 12, 2024

	origins: Dict[str, SourceOperator]
	origins: Union[List[SourceOperator], Dict[str, SourceOperator]]

		@@ -1560,6 +1560,48 @@ def process(self, multi_stream: MultiStream) -> MultiStream:
		return MultiStream(result)


		class SplitByGroup(MultiStreamOperator):

	class SplitByGroup(MultiStreamOperator):
	class SplitByNestedGroup(MultiStreamOperator):
	field: str = "group"

fuse, split, MultiStreamScoreMean, and merge all #767

fuse, split, MultiStreamScoreMean, and merge all #767

Conversation

dafnapension commented Apr 13, 2024 • edited

elronbandel Apr 14, 2024 • edited

Choose a reason for hiding this comment

dafnapension Apr 14, 2024 • edited

Choose a reason for hiding this comment

elronbandel Apr 14, 2024

Choose a reason for hiding this comment

dafnapension Apr 14, 2024 • edited

Choose a reason for hiding this comment

dafnapension Apr 15, 2024

Choose a reason for hiding this comment

elronbandel Apr 14, 2024

Choose a reason for hiding this comment

dafnapension Apr 14, 2024

Choose a reason for hiding this comment

dafnapension Apr 15, 2024

Choose a reason for hiding this comment

elronbandel left a comment • edited

Choose a reason for hiding this comment

dafnapension commented Apr 14, 2024 • edited

codecov bot commented May 5, 2024 • edited

Codecov Report

elronbandel May 8, 2024

Choose a reason for hiding this comment

dafnapension commented May 11, 2024

elronbandel May 12, 2024

Choose a reason for hiding this comment

dafnapension commented Apr 13, 2024 •

edited

elronbandel Apr 14, 2024 •

edited

dafnapension Apr 14, 2024 •

edited

dafnapension Apr 14, 2024 •

edited

elronbandel left a comment •

edited

dafnapension commented Apr 14, 2024 •

edited

codecov bot commented May 5, 2024 •

edited