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
Search Result Explanation Feature #3069
Conversation
Thanks for sharing this, @ThoWagen! I'm putting it in the 10.0 milestone, since I think it is unlikely that this will be fully reviewed and revised in time for release 9.1, but I want to be sure we don't lose track of it. I'm presently very busy trying to finish up 9.1-related work, but I will try to circle back and give this a first review as soon as it is practical to do so! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, @ThoWagen! I haven't had time for a full review yet, but I took a few minutes today to resolve conflicts here and to begin to look over the code. See below for a few initial comments. I will add more as I have time to review more closely.
languages/ExplainFieldName/de.ini
Outdated
@@ -0,0 +1,4 @@ | |||
;description of fields in explain | |||
allfields_unstemmed = "Alle Felder" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice that the English strings mention copyField, but the German ones do not. Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this. Hopefully this is better understandable now. But as mentioned in the initial description I am not sure about those field explanations. For now these are just three examples. Someone with more experience with the standard VuFind index could/should have a look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I'm seeing this error in JS console in search results:
Uncaught TypeError: columnChart is null
_setupExplainColumnChart http://localhost/vufind2/themes/bootstrap3
/js/explain.js?_=1699432031:137
- I'd prefix any class used as selector in JS with
js-
. This is not consistent in the code base at the moment, but a practice I'd recommend. - How would you feel about making the score canvas the link to explain in results list by default and adding a description to the title attribute or something? I'm afraid the simple "Explain" link could be confusing, and "Explain the score" or such would be a bit long.
- Nit: there's a bit of inconsistency in formatting of comments. I'd ensure there's a space after the '//' everywhere. Also I believe the prevalent style in the code base is to start with a capital letter.
themes/bootstrap3/templates/RecordDriver/DefaultRecord/result-list-explain.phtml
Outdated
Show resolved
Hide resolved
@EreMaijala I fixed the JS error, added I am not sure about the link in the result list. I agree that "explain" alone might be confusing. But if we only have the chart with mouseover text then it might not be clear that you can interact with it. What do you think about this @demiankatz? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of replies below, along with a small issue I noticed while reviewing.
Regarding the UI question, what if instead of the "Explain" text, we put an icon next to the score bar, like a little question mark? Then we could give the bar alt text/labeling to say something like "Score: N" and label the icon with "Explain score" (or something similar). What do you think?
module/VuFind/src/VuFind/Search/Explanation/ExplanationFactory.php
Outdated
Show resolved
Hide resolved
That's a good idea! |
I implemented @demiankatz suggestion for the result list link and added support for Search2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EreMaijala I implemented something for synonyms and it works with the default Solr index for me. Could you confirm that it works for you too? If yes I will refactor and clean up the code. I also ran the Mink Tests with explain enabled and some of them failed. I don't think that should happen and will have a look into this next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Synonyms look good to me now. I'll just mark this as "Requested changes" until you've done the minor fixes and cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me, just one more little thing.
themes/bootstrap3/templates/RecordDriver/DefaultRecord/explain-line.phtml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was asked to view the explanation features in all three themes. I found that the display was consistent all three themes, in both the search results summary screen and in each page that detailed the explanation.
Everything worked as expected, as far as I can tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, everyone, for keeping this moving forward! I haven't finished a full review of this yet myself (running short on time before the holidays), but I was able to look through about half of the files here, and I had a couple of minor questions. See below...
module/VuFind/src/VuFind/RecordDriver/SolrDefaultWithoutSearchServiceFactory.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the progress on this! I managed to finish reading through all of the files, and I found a few more minor things to fix.
One other general issue: I'm concerned that in the long term, this code may be fragile if Solr changes its output format. I realize that there's no perfect way to test for this, but I wonder if some kind of Mink test might be worth developing, to at least make sure that explanation returns some kind of values and not an error message or garbage. It's obviously impossible to write a test that will reliably get the same scores all the time, but maybe something that just confirms we're getting numbers where we expect numbers, etc., would be enough to catch the most severe of errors. What do you think?
module/VuFind/src/VuFind/RecordDriver/Search2DefaultFactory.php
Outdated
Show resolved
Hide resolved
themes/bootstrap3/templates/RecordDriver/DefaultRecord/explain-line.phtml
Outdated
Show resolved
Hide resolved
I added a Mink test class that tests that at least some results are displayed and are not empty. I agree that it probably is not possible to test the exact values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, @ThoWagen.
Before I merge, just one last question: do you think it's worth investing time in adding more descriptions to the IndexFieldDescription text domain? If you think it would be helpful, I'm certainly willing to help with this. That being said, though, I wonder if this is worth having at all -- right now, since they are only used in a title attribute, it is fairly difficult to even see these descriptions or realize that they are available. Do you anticipate other uses for these translations, or is it possible that the work of maintaining and translating them is greater than the value they provide?
We want to use the descriptions but use other fields in our index. So I can't simply copy them in this PR. We could simply use the descriptions from here https://vufind.org/wiki/development:architecture:solr_index_schema. Or we don't include any translations is this PR at all but still include the logic so that anyone can add their own descriptions. I don't have strong feelings about this. I can also see that it's not obvious that the description exists. We also add a small hoverable question mark here. |
Thanks, @ThoWagen. I agree that it might be valuable to add a hoverable question mark icon when a description is set -- maybe we can make that last change before merging this, if you have time. Beyond that, I would suggest that we just leave the translations that are currently here as examples but don't add more yet (so as not to create significant amounts of work for translators on a feature that may be rarely used). If there is interest/demand, we can always expand them in future, using the wiki page as a starting point as you suggest. |
@demiankatz I added the question mark and agree with your take on the translation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ThoWagen, this looks good to me now!
It is not always obvious why a record was found or how it is ranked in the result list. Solr can give you an explanation for this but it is far from easy to understand on first glance. This feature allows to view this explanation in a more readable and visual format. It can help librarians, developers to optimize the searchspecs.yaml or help them and even advanced users to better understand the search results. This feature was presented at Wolfcon 2023.
It can be activated by enabling it under [Explain] in the searches.ini and should work with all Solr versions. However, the format of Solr's explanation varies from version to version and it might be a good idea to test this on multiple systems and with different indexes.
Note: Under languages/ExplainFieldName, descriptions can be included for fields in Solr that might not be obvious. This obviously depends on the index schema. So for now, I only included three fields as an example. I am not sure if and which fields of the standard VuFind schema would need a description.