Bug fixes #23
Bug fixes #23
Conversation
| @@ -213,8 +213,6 @@ | |||
| "results": { "type": "object" }, | |||
| "unique": { "type": "boolean" } | |||
| }, | |||
|
|
|||
| "required": ["class"], | |||
There was a problem hiding this comment.
why are we removing the "class" requirement?
This requirement is specified by the API docs.
If we don't require, we need to make sure we are handling blobs/images/descriptors/videos well as well, when, say, the results block has the "return_blob" flag.
We should probably think more about this before removing the requirement, as it introduces a lot of undefined behavior.
There was a problem hiding this comment.
I am not sure how those are correlated. class is what we have for the tag in PMGD. It should not affect for blobs. You could give a class for an entity that has no blob and still ask for return blob. For images, descriptors etc, it is counter intuitive to expect class since those are implicit anyway. What am i missing here?
There was a problem hiding this comment.
so, If we do findEntity with no class, we need to filter out those entities that are images/descriptors/videos?
There was a problem hiding this comment.
Why? Aren't they regular nodes with properties in the graph? It does affect dealing with blob some cause technically you have blobs associated with them and if someone asked for a blob, I am not sure the corresponding property names would match.
There was a problem hiding this comment.
and if that is the case, we need to fix findEntity to do that.
I would leave it required until we have the necessary mechanism to deal with it.
There was a problem hiding this comment.
I am fine doing it. It is an unnecessary limitation. Besides, blob is the one not checked in yet and so it is possible to handle it. This is already in our repo. But it is a useful capability to be able to search across all nodes without class barrier.
There was a problem hiding this comment.
Also this leaves us a more complicated way of enumerating all entities. Another reason to remove the restriction. And an unrelated note: we should probably have some way of returning the class of an entity
There was a problem hiding this comment.
I see. You convinced me. Makes sense.
I will update the wiki page and remove the requirement.
| @@ -190,7 +190,7 @@ | |||
| "Age": [">", 0, "<=", 100 ] | |||
| }, | |||
| "results": { | |||
| "list":["Name","Age","Email"], | |||
| "list":["Name","Age","Email", "Study"], | |||
There was a problem hiding this comment.
Adding this is not checking whether the response is correct.
We should add a tests that checks whether we are getting the right response.
I think we should move this tests to python, instead of using this file and C++ (not for this, but for another different pull request for later)
There was a problem hiding this comment.
Yeah I did my usual thing of visual inspection 😀
Luis, try this out with the TCIA queries. I don't have that graph to test if some of the issues you were seeing have now been fixed.