Skip to content
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

Semantic search, rough but functional #471

Merged
merged 47 commits into from
Jun 2, 2021
Merged

Conversation

jgonggrijp
Copy link
Member

@jgonggrijp jgonggrijp commented May 31, 2021

This is the finalization of #455, following up on #465 and #468. The welcome view (/search route) now has "Text search" and "Semantic search" tabs. The former is open by default and shows the pre-existing full-text search box. The latter reveals the recursive linked data traversal/narrowing/filtering form that I previously demonstrated. When clicking the "Search" button in this tab, the explorer is shown with a single search results panel, using ItemSummaryBlock to represent each result item. Clicking one opens an AnnotationPanel with the item. From here on, users can follow the annotations, related items etcetera.

There are some rough edges, which I decided not to address yet so that we could deploy sooner:

  • Text overflow glitch on the semantic search dropdowns. This doesn't always happen, and it might be specific to Safari.
  • (Synthetic?) Reverse properties in the dropdowns sometimes lack a label. I suspect this is caused by the hierarchy problem in the new ontology, rather than by the semantic search feature.
  • Queries cannot be saved yet and the search results show with an incorrect route. I plan to address these issues together in a later PR, by saving semantic queries to the backend (postgres, not fuseki) and giving them a numerical ID.

I merged develop into my feature branch in order to test the feature with the new ontology. It worked more or less as-is, but I could not access pre-existing data that were migrated from the skinny ontology because they were associated with superclasses. I also needed to remove the restriction that only leaf classes can be selected in order to be able to follow inverse properties; this is something we'll need to discuss with @fvignale on Tuesday.

The final commit in this PR (at the time of writing), 77ec7f4, allows for the selection of superclasses. We could omit/revert this commit in order to stick with the more restrictive subclass-only semantics. Otherwise, a couple of unittests will need slight updates, but this could wait until after merging.

For the review, I suggest the following:

@BeritJanssen

  • Read the diff of the changed sources outside of the frontend/src/semantic-search directory.
  • Read a local checkout of the frontend/src/semantic-search directory.

@JeltevanBoheemen

  • Read the diff of the changed sources outside of the frontend/src/semantic-search directory.
  • Test the feature locally.

jgonggrijp added 30 commits May 19, 2021 16:44
Copy link
Member

@BeritJanssen BeritJanssen left a comment

Choose a reason for hiding this comment

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

The code looks clear to me. I think that the semantic-search components should have more documentation: what is a chain, how does it go together with multibranch? Perhaps this could also be clarified in the README / search documentation. Right now it might not be clear a developer from outside of the project which DOM elements correspond to which views.

@jgonggrijp
Copy link
Member Author

jgonggrijp commented Jun 1, 2021

Thanks @BeritJanssen. I will add more documentation to the views and also add a (short) dedicated README to the frontend/src/semantic-search directory. (At a later time after merging.)

@jgonggrijp jgonggrijp mentioned this pull request Jun 1, 2021
2 tasks
Copy link
Contributor

@JeltevanBoheemen JeltevanBoheemen left a comment

Choose a reason for hiding this comment

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

Code looks clean, interested in how the sparql queries are built, but will do this later :)

Seems I encounter a bug. When making a simple query (Name - is defined), the result is route http://localhost:8000/explore/item//annotations and shows:
Screenshot 2021-06-01 at 17 21 34
The empty squares are actual items, for example the TextQuoteSelector corresponding with the correct annotation.

Console logs errors:

The select2('open') method was called on an element that is not using Select2. select2.js:6062
Uncaught TypeError: Cannot read property 'open' of undefined select2.js:6068

Also, only partially related, the labels are going wild with languages. I believe you are already aware of this.
Screenshot 2021-06-01 at 17 31 53

@jgonggrijp
Copy link
Member Author

Thanks for the review @JeltevanBoheemen! The query that you tried is invalid, i.e., the form is not meant to be used that way. So the output is "expected", but the form is still lacking the validation needed to prevent people from submitting such a query. Again, clear evidence that usage is not self-evident.

You can get output that doesn't look so buggy by interjecting a property traversal. (The general rule is that the form as a whole must contain at least one property traversal, and that every chain must end in a filter.)

Do you think the validation should be added before we can merge this?

The incorrect route is one of the rough edges I mentioned in the OP.

@JeltevanBoheemen
Copy link
Contributor

I think the validation should be in place before deploying to production. I do see the value in testing this out on acc before that though (rough edged as it may be). So I'd say merge.

@jgonggrijp jgonggrijp merged commit eaa1345 into develop Jun 2, 2021
@jgonggrijp jgonggrijp deleted the feature/semantic-search branch June 2, 2021 01:04
This was referenced Jun 2, 2021
@jgonggrijp jgonggrijp added this to the Next release milestone Jun 16, 2021
jgonggrijp added a commit that referenced this pull request Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants