Skip to content

Explanation Conclusion should include non-retrievable concepts #157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 25, 2022

Conversation

jmsfltchr
Copy link
Contributor

@jmsfltchr jmsfltchr commented May 24, 2022

What is the goal of this PR?

Introduce a different answer type for conclusions inside explanations. This permits returning any concept, not only retrievable concepts, which is important for returning a thorough explanation due to unification.

What are the changes implemented in this PR?

  • Swap ConceptMap for a map

Comment on lines 124 to 126
message ConclusionAnswer {
map<string, Concept> map = 1;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message/object is not meaningful given that it wraps one object -- it's a thin wrapper and not canonical. We should keep a simple <map, Concept> message type for conclusion field in Explanation message type.

@jmsfltchr jmsfltchr requested a review from haikalpribadi May 25, 2022 10:08
@jmsfltchr jmsfltchr changed the title ConclusionAnswer for Explanations Explanation Conclusion should include non-retrievable concepts May 25, 2022
@haikalpribadi haikalpribadi merged commit 84a1eb7 into typedb:master May 25, 2022
@jmsfltchr jmsfltchr deleted the reactive-architecture branch May 25, 2022 10:19
haikalpribadi added a commit that referenced this pull request May 25, 2022
@haikalpribadi
Copy link
Member

I've revered this PR in a subsequent commit. Reason is the same as what I posted on Client Java's PR:

We've concluded that this change, along with the Protocol prerequisite change, does not arrive at meaningful data structure. Replacing ConceptMap with Map<Str,Concept> was not meaningful given that we still provide users with the same information but with different APIs. The only change is that the users are now restricted from calling .explanations() but that wasn't bad to begin with, given that the return was empty. Using ConceptMap, albeit providing richer API (with .explanations()) than necessary was not bad given that it's still consistent. We think the there's more to this discussion, in that we need to: a) look at the overall design of ConceptMap with with a fresh perspective, and b) review more of what reasoner's new data structure actually needs. Both will need to happen in the future, and thus for now, we should stick with the current API we have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants