-
-
Notifications
You must be signed in to change notification settings - Fork 280
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
fix: #1044 - now using a unique compatibility algorithm #1046
fix: #1044 - now using a unique compatibility algorithm #1046
Conversation
Impacted files: * `app_en.arb`: added a short "compatible" label; down to 3 longer compatibility labels * `app_fr.arb`: added a short "compatible" label; down to 3 longer compatibility labels * `product_compatibility_helper.dart`: removed all computations; down to 3 compatibility possibilities * `smooth_product_card_found.dart`: now using `MatchedProduct` and its computations * `summary_card.dart`: now using `MatchedProduct` and its computations
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.
The code looks good, and I am in favour of keeping one algorithm but let's wait for other opinions on that
|
||
// Defines the weight of an attribute while computing the average match score | ||
// for the product. The weight depends upon it's importance set in user prefs. | ||
const Map<String, int> attributeImportanceWeight = <String, int>{ |
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.
so this was an algorithm written by Stephane to compute the product compatibility score, does MatchedProduct
adhere to the same algorithm ? I'm not sure, but it doesn't seem like it, and it should :)
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.
What we can agree on is that we should use only one algorithm.
@teolemon seemed to say that 5 levels were considered by end-users as too confusing and that 3 are enough.
And my algorithm too was initially written by Stéphane, I just converted it to dart.
Which algorithm should we keep? No idea.
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.
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.
@stephanegigandet @teolemon @jasmeet0817 @M123-dev My opinion: the main point here is to have a unique score algorithm on the app (much better for end-users, UX-wise), and that is implemented by this PR.
We can later say: maybe we should improve the score algorithm, add a possible enum value, fix here and there.
But for the moment, that looks good enough.
I agree with the simplification in this PR, and specially getting rid of the % match and converting that to simply "Compatible/Incompatible...". Which is simple, and simple is always good. My only concern is around the computation of the compatibility. Stephane described the algorithm in "Personal Match Banner -> Alternative match algorithm proposal" section. |
@monsieurtanuki Yes, I pinged @stephanegigandet about that, to double check the formula, but we're all busy on a packaging input related question. I'm going to approve it 👍 |
Yes agree and that's the advantage of git we'll always have access to the current implementation |
Impacted files:
app_en.arb
: added a short "compatible" label; down to 3 longer compatibility labelsapp_fr.arb
: added a short "compatible" label; down to 3 longer compatibility labelsproduct_compatibility_helper.dart
: removed all computations; down to 3 compatibility possibilitiessmooth_product_card_found.dart
: now usingMatchedProduct
and its computationssummary_card.dart
: now usingMatchedProduct
and its computationsWe also got rid of the
129% compatibility
label, replaced by'compatible'