Use a default vocabulary ID when guessing vocabulary, to save lookups #851
Conversation
|
Tried to fix the merge conflict using the GitHub UI but it didn't seem to work. Trying again locally... |
osma
force-pushed
the
issue836-guess-vocab-default
branch
from
41a73a8 to
50c5e08
Compare
|
Fixed the conflict locally, rebased over current master, squashed the two commits into one. |
There was a problem hiding this comment.
Tested locally with Docker, both appear to give same results in about the same time, without noticeable difference. But I think I am simply not using a good test case.
I started searching by "finland" in both. As there is the place Finland in YSO-Places, and several concepts that start with Finland in the YSO-General.
Then after that I tried searching for "Ähtäri", in YSO-Places, which has mapping concepts from YSO-General.
But maybe in a real server - or with the correct test data - the result will be better. Code looks good tho, so 👍
model/Model.php
Outdated
| $realvoc = $this->guessVocabularyFromURI($hit['uri']); | ||
| if ($realvoc !== $voc) { | ||
| $realvoc = $this->guessVocabularyFromURI($hit['uri'], $voc !== null ? $voc->getId() : null); | ||
| if ($realvoc != $hitvoc) { |
There was a problem hiding this comment.
Maybe safer to go with !==? (though != should work here I think)
There was a problem hiding this comment.
Ah right, good catch! I think you changed the operator recently and this was probably the source of the merge conflicts...
|
The effect should be most noticeable for queries in YSO (or YSO places) that return a lot of results. For example short strings like "aut" or "kar". In those cases currently Skosmos has to do a bit of extra work to match each concept to a vocabulary. This patch should reduce that work and thus improve the response time for generating the search result page. Will test this soon... |
|
YSO (Finnish) search for "aut" (89 hits) goes from 2.4 seconds to 1.0 seconds. As speculated above, the speedup is most noticeable for long search result lists. The one problematic case I can think of is YSO concepts that link to YSO places, for example Johanneksenkirkko (Helsinki). Currently the link is to Helsinki in YSA but I suspect it will be changed soon to point to YSO Places as YSA is being deprecated. I will have to test this separately before merging... |
|
It turns out the display of (non-mapping) links to YSO Places concepts on YSO concept pages is already broken:
(The place is shown as This PR doesn't change the situation. |
|
Reported the concept page issue as #868. |
Part of #836
This change makes better use of the
defaultVocabIdparameter inguessVocabularyFromURImethod in two places. When handling search results and concept properties, the ID of the current vocabulary is used as default so that when a URI that is within the URI space of current vocabulary (e.g. YSO) is encountered, it is assumed to be within the current vocabulary, even though theoretically it could be from another vocabulary (e.g. YSO Places) which uses the same URI namespace.This may be a little bit risky, for example if a YSO concept links to a YSO Places concept using a property that is not a mapping property.