Implement merge-concrete-entities function and add some tests #153

bmerkle · 2026-01-05T19:20:11Z

first implementation of missing merge-concrete-entities function
please have a look if the semantic and implementation is ok.
if yes, we could also add some further test cases.

gvanrossum · 2026-01-05T22:59:26Z

I'm too tired to review this evening but at least all the tests pass.

gvanrossum

This ought to be consistent with the TS version at
https://github.com/microsoft/TypeAgent/blob/120e280c7b2acfeb499b5bbc5d78cea23f75b531/ts/packages/knowPro/src/knowledgeMerge.ts#L144C8-L144C38

I'm experimenting with copilot review -- hopefully it can do the check for us.

src/typeagent/knowpro/knowledge.py

Copilot

Pull request overview

This PR implements the previously unimplemented merge_concrete_entities function, which merges a list of concrete entities by name, combining their types and facets. The implementation includes comprehensive test coverage with 9 test cases covering various merging scenarios.

Replaces NotImplementedError stub with working implementation using a dictionary-based merging strategy
Adds helper classes and functions (_MergedEntity, _facets_to_merged, _merged_to_facets) to support the merging logic
Implements comprehensive test suite covering edge cases including empty lists, single entities, distinct entities, case sensitivity, type deduplication, and facet merging

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
src/typeagent/knowpro/knowledge.py	Implements `merge_concrete_entities` function with helper classes and functions for merging entities by name, combining types (sorted and deduplicated) and facets (concatenated with "; " separator)
tests/test_knowledge.py	Adds 9 comprehensive test cases covering various merge scenarios including empty inputs, single entities, distinct entities, case sensitivity, type/facet merging, and deduplication

src/typeagent/knowpro/knowledge.py

tests/test_knowledge.py

src/typeagent/knowpro/knowledge.py

gvanrossum · 2026-01-06T23:09:31Z

@copilot Please review this function carefully. It should be equivalent in semantics to the TypeScript function mergeConcreteEntities defined here:
https://github.com/microsoft/TypeAgent/blob/120e280c7b2acfeb499b5bbc5d78cea23f75b531/ts/packages/knowPro/src/knowledgeMerge.ts#L144C8-L144C38

gvanrossum · 2026-01-06T23:12:12Z

@bmerkle Feel free to ignore or implement Copilot's suggestions (or let it implement some itself). I am still waiting for it to compare the Python version to the TypeScript version.

refactored based on @copilot suggestion Co-authored-by: Copilot <[email protected]>

- adopted implementation and tests to be caseinsensitive (as in the TS reference app)

…lity

gvanrossum · 2026-01-07T18:44:52Z

src/typeagent/knowpro/knowledge.py

+@dataclass
+class _MergedEntity:
+    """Internal helper for merging entities."""
+
+    name: str
+    types: set[str]
+    facets: dict[str, set[str]]


I'd move this before merge_concrete_entities.

My general ideal ordering is: functions top-down, but classes ahead of functions (or at least ahead of the first function that references it).

gvanrossum · 2026-01-07T19:17:01Z

src/typeagent/knowpro/knowledge.py

+def _add_facet_to_merged(
+    merged: dict[str, set[str]], facet: kplib.Facet
+) -> None:


My version of black collapses this to one line.

gvanrossum · 2026-01-07T19:22:06Z

src/typeagent/knowpro/knowledge.py

 def merge_concrete_entities(
    entities: list[kplib.ConcreteEntity],
 ) -> list[kplib.ConcreteEntity]:


I wonder if we could make the lower-casing optional, controlled by an optional argument. That argument could be a Callable[[str], str] and have a default of str.lower. (That should work -- or otherwise lambda x: x.lower().)

gvanrossum · 2026-01-09T19:08:08Z

src/typeagent/knowpro/knowledge.py

+        if existing is None:
+            # First occurrence - create new merged entity
+            merged[name_key] = _MergedEntity(
+                name=entity.name.lower(),


Suggested change

name=entity.name.lower(),

name=name_key,

gvanrossum · 2026-01-09T19:14:06Z

src/typeagent/knowpro/knowledge.py

+
+    Complex types like Quantity and Quantifier use their __str__ representation.
+    """
+    return str(value).lower() if value else ""


This would return "" if the value is 0 or 0.0.

Suggested change

return str(value).lower() if value else ""

return str(value).lower() if value is not None else ""

gvanrossum · 2026-01-09T19:19:07Z

src/typeagent/knowpro/knowledge.py

+def _merged_to_facets(merged_facets: dict[str, set[str]]) -> list[kplib.Facet]:
+    """Convert a merged facets dict back to a list of Facets."""
+    facets = []
+    for name, values in merged_facets.items():


Maybe also sort the names?

Suggested change

for name, values in merged_facets.items():

for name, values in sorted(merged_facets.items()):

Or maybe

Suggested change

for name, values in merged_facets.items():

for name in sorted(merged_facets):

values = merged_value[name]

gvanrossum · 2026-01-09T19:22:09Z

src/typeagent/knowpro/knowledge.py

+        if existing is None:
+            # First occurrence - create new merged entity
+            merged[name_key] = _MergedEntity(
+                name=entity.name.lower(),
+                types=set(t.lower() for t in entity.type),
+                facets=_facets_to_merged(entity.facets) if entity.facets else {},
+            )
+        else:
+            # Merge into existing
+            existing.types.update(t.lower() for t in entity.type)
+            if entity.facets:
+                _merge_facets(existing.facets, entity.facets)


Consider creating only an "empty" _MergedEntity and taking the else flow always. Then you could drop _facets_to_merged().

Suggested change

if existing is None:

# First occurrence - create new merged entity

merged[name_key] = _MergedEntity(

name=entity.name.lower(),

types=set(t.lower() for t in entity.type),

facets=_facets_to_merged(entity.facets) if entity.facets else {},

)

else:

# Merge into existing

existing.types.update(t.lower() for t in entity.type)

if entity.facets:

_merge_facets(existing.facets, entity.facets)

if existing is None:

# First occurrence - create new merged entity

merged[name_key] = _MergedEntity(

name=name_key,

types=set(),

facets={},

)

# Merge into existing

existing.types.update(t.lower() for t in entity.type)

if entity.facets:

_merge_facets(existing.facets, entity.facets)

gvanrossum · 2026-01-09T19:34:46Z

src/typeagent/knowpro/knowledge.py

+    facets: dict[str, set[str]]
+
+
+def _facet_value_to_string(value: kplib.Value | None) -> str:


I'd get rid of this function and just inline it.

Implement merge_concrete_entities function and add some tests

2197f9c

bmerkle temporarily deployed to build-pipeline January 5, 2026 22:51 — with GitHub Actions Inactive

gvanrossum reviewed Jan 6, 2026

View reviewed changes

src/typeagent/knowpro/knowledge.py Show resolved Hide resolved

gvanrossum marked this pull request as draft January 6, 2026 22:13

This comment was marked as duplicate.

Sign in to view

gvanrossum requested a review from Copilot January 6, 2026 22:35

Copilot started reviewing on behalf of gvanrossum January 6, 2026 22:36 View session

Copilot AI reviewed Jan 6, 2026

View reviewed changes

bmerkle and others added 2 commits January 7, 2026 00:58

Update src/typeagent/knowpro/knowledge.py

7d96468

refactored based on @copilot suggestion Co-authored-by: Copilot <[email protected]>

- improved implementation based on copilot feedback in review

b36885a

- adopted implementation and tests to be caseinsensitive (as in the TS reference app)

bmerkle temporarily deployed to build-pipeline January 7, 2026 03:09 — with GitHub Actions Inactive

bmerkle added 2 commits January 8, 2026 11:50

Merge branch 'main' into merge-concrete-entities

ae966d0

Refactor _add_facet_to_merged function signature for improved readabi…

c4f1c1f

…lity

gvanrossum reviewed Jan 9, 2026

View reviewed changes

	return str(value).lower() if value else ""
	return str(value).lower() if value is not None else ""

	for name, values in merged_facets.items():
	for name, values in sorted(merged_facets.items()):

	for name, values in merged_facets.items():
	for name in sorted(merged_facets):
	values = merged_value[name]

		facets: dict[str, set[str]]


		def _facet_value_to_string(value: kplib.Value \| None) -> str:

Implement merge-concrete-entities function and add some tests #153

Are you sure you want to change the base?

Implement merge-concrete-entities function and add some tests #153

Uh oh!

Conversation

bmerkle commented Jan 5, 2026

Uh oh!

gvanrossum commented Jan 5, 2026

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as duplicate.

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gvanrossum commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jan 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gvanrossum commented Jan 6, 2026 •

edited

Loading