-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
feat: Synonyms in taxonomized suggestions #9395
feat: Synonyms in taxonomized suggestions #9395
Conversation
The values, both underlying and displayed, are currently broken
This is still a prototype, and the values shown are still broken. The synonyms should be sent by the display format rather than the ID. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #9395 +/- ##
========================================
Coverage 49.54% 49.55%
========================================
Files 67 67
Lines 20650 20769 +119
Branches 4980 5001 +21
========================================
+ Hits 10231 10292 +61
- Misses 9131 9187 +56
- Partials 1288 1290 +2 ☔ View full report in Codecov by Sentry. |
Here is a demo showing the current progress. I haven't yet figured out how to return the synonyms' names properly. That should be done on the server side which has the full taxonomy information. Synonym.Suggestions.Prototype.Showcase.mp4Here is the current result of {"errors":[],"matched_synonyms":["orge","oeufs","arachis-hypogaea","langoustine","fruits-a-coque-dure","beurre-concentre","petoncles","graines-de-moutarde","rouget","tofu"],"status":"success","suggestions":["Gluten","Œufs","Arachides","Crustacés","Fruits à coque","Lait","Mollusques","Moutarde","Poisson","Soja"],"warnings":[]} |
The response from {"errors":[],"matched_synonyms":["Orge","Œufs","Arachis hypogaea","Langoustine","Fruits à coque dure","Beurre concentré","Pétoncles","Graines de moutarde","Rouget","Tofu"],"status":"success","suggestions":["Gluten","Œufs","Arachides","Crustacés","Fruits à coque","Lait","Mollusques","Moutarde","Poisson","Soja"],"warnings":[]} Which can be transformed like this:
|
f5d8eb5
to
8047944
Compare
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.
@Naruyoko: thanks a lot for this PR.
I have a blocker though. There are other people relying on this suggestion API. So we must not change the response format.
To better play with OpenAPI, I propose that we do a suggestion2.cgi API with your new format (@stephanegigandet do you have a better idea ?)
@@ -247,6 +247,11 @@ paths: | |||
description: Array of sorted strings suggestions in the language requested in the "lc" field. | |||
items: | |||
type: string | |||
matched_synonyms: |
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.
from what I undertand seeing tests, you did not only change api-v3 but also api-v2 (in api.yml). shan't you change it there also ?
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 comment might not be relevant if we go for a suggestion api v2.
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 am not sure where I should separate my changes. Do I copy this part with a new name with the new function exposed only there? If so, do I need to copy this function as well? Or do the changes need to be separated at the level of inner functions, like from here?
I am also not sure how I should change api.yml
, since it appears mostly empty.
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.
@Naruyoko in the code you show, condition should be enough to sort out the cases.
But we could add a parameter to get_taxonomy_suggestions
and have a cgi/suggest_v2.pl
alike suggest.pl
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.
@Naruyoko, do not hesitate to ping me on slack if it's not clear enough.
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.
By using dict, do you mean like this (which was what I intended by "traspose"):
{"errors":[],"suggestions":[{"matched_synonym":"Orge","tag":"Gluten"},{"matched_synonym":"Œufs","tag":"Œufs"},{"matched_synonym":"Arachis hypogaea","tag":"Arachides"},{"matched_synonym":"Langoustine","tag":"Crustacés"},{"matched_synonym":"Fruits à coque dure","tag":"Fruits à coque"},{"matched_synonym":"Beurre concentré","tag":"Lait"},{"matched_synonym":"Pétoncles","tag":"Mollusques"},{"matched_synonym":"Graines de moutarde","tag":"Moutarde"},{"matched_synonym":"Rouget","tag":"Poisson"},{"matched_synonym":"Tofu","tag":"Soja"}],"warnings":[]}
Or like this:
{"errors":[],"suggestions":{"Gluten":"Orge","Œufs":"Œufs","Arachides":"Arachis hypogaea","Crustacés":"Langoustine","Fruits à coque":"Fruits à coque dure","Lait":"Beurre concentré","Mollusques":"Pétoncles","Moutarde":"Graines de moutarde","Poisson":"Rouget","Soja":"Tofu"},"warnings":[]}
Or like this:
{"errors":[],"matched_synonyms":{"Gluten":"Orge","Œufs":"Œufs","Arachides":"Arachis hypogaea","Crustacés":"Langoustine","Fruits à coque":"Fruits à coque dure","Lait":"Beurre concentré","Mollusques":"Pétoncles","Moutarde":"Graines de moutarde","Poisson":"Rouget","Soja":"Tofu"},"status":"success","suggestions":["Gluten","Œufs","Arachides","Crustacés","Fruits à coque","Lait","Mollusques","Moutarde","Poisson","Soja"],"warnings":[]}
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 like your third version, because it keeps things simple as you can just retrieve the simple suggestion list, and it's easy to find back matched synonym.
Just one point: shan't it be "matched_synonyms":{"Gluten":["Orge"],…}
as there might be more than one matched synonym (or don't we care because we just return one ? Which is still really ok for me, as it covers the needs).
Also is it useful to put entries like "Œufs": "Œufs" inside matched_synonyms or can the API user deduce from the fact that Œufs is not in the synonyms dict that it means it was directly matched ?
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 agree with using the third version.
The matched synonym is currently the first matched one of the best match (with "start">"inside">"fuzzy"). I don't see how much useful it is to return all possible matches.
I think it is better to keep "a":"a"
entries for consistency.
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 pushed a change with the new format. I will have to update the API docs and create new tests.
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.
Yes, I read your changes, seems perfect 💪
So I wait for the other changes. Don't hesitate to ping me on slack when you are ready @Naruyoko :-)
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.
Very good @Naruyoko, thanks for this great contribution !
I add a very small change to documentation.
Quality Gate passedIssues Measures |
When creating suggestions for taxonomized fields like categories, the API gives back results where one of the synonyms match. However, they are not displayed when they don't include the typed string. This tries to fix that by returning which synonym the input matched to build the suggestions list.
Make sure you've done all the following (You can delete the checklist before submitting)
What
When creating suggestions for taxonomized fields like categories, the API gives back results where one of the synonyms match. However, they are not displayed when they don't include the typed string. This tries to fix that by returning which synonym the input matched to build the suggestions list.
Related issue(s) and discussion