Allow changing the weight given to classifying conditions #179
Conversation
mariushoch
force-pushed
the
bug/T132839
branch
from
fd3769f
to
6f73c24
Compare
| $condition = $dbr->makeList( $idTuples, LIST_OR ); | ||
|
|
||
| $dbr = $this->lb->getConnection( DB_REPLICA ); | ||
| if ( $hasClassifyingPropertyIds && $this->classifyingConditionWeight != 0.5 ) { |
There was a problem hiding this comment.
I forgot about this… I actually wanted to replace this with a safer float comparison.
mariushoch
force-pushed
the
bug/T132839
branch
from
6f73c24
to
8a38ac6
Compare
There was a problem hiding this comment.
I do have quite some suggestions, but at the same time I think we should move forward, merge this, experiment and allow us to possibly fail. It's a setting. The theory is that we can simply change this setting back to the default, and everything will be back to as it was before. Right?
| // and classifyingConditionWeight is not 0.5. | ||
| $propSelect = $this->getWeightedPropSelect( $dbr->getType(), $count ); | ||
| } else { | ||
| $propSelect = "sum(probability)/$count"; |
There was a problem hiding this comment.
Isn't this duplicate code? See below.
| * @return string | ||
| */ | ||
| private function getWeightedPropSelect( $dbType, $count ) { | ||
| // The sum of the weights needs to be 2. |
There was a problem hiding this comment.
Why does it need to be 2? I believe I understand, but the comment should explain.
| * @return Suggestion[] | ||
| */ | ||
| protected function getSuggestions( array $propertyIds, array $idTuples, $limit, $minProbability, $context ) { |
There was a problem hiding this comment.
It looks like the removal of this parameter could easily be a separate patch. Splitting this into more smaller patches would help a lot in getting reviews for this significant change. Same for the removal of protected. I'm willing to help and upload such patches, if there is a chance they get merged fast.
|
|
||
| ## Release notes | ||
|
|
||
| ### 4.0.0 (dev) |
There was a problem hiding this comment.
I wonder why this additional setting requires such a major version bump? As far as I can tell this is not a breaking change.
|
Please note: This is not yet ready to be merged: There is still an open design question which I need to properly encode in configuration or decide (with the community and the PM) before merging. |
Allow changing the weight given to classifying conditions via the $wgClassifyingConditionWeight setting. This only applies to the "item" context if there is at least one classifying statement. During testing it turned out that the classifying conditions resulted in way better suggestions than the ones which are purely based on whether a Statement with a given Property id exists. Due to this, I want to be able to boost the weight of these classifying conditions, in order to address T132839. Note: This is only supported on MySQL and SQLite right now, like the rest of this extension. On other DBMS, the setting will not be taken into account. https://phabricator.wikimedia.org/T132839
|
I manually rebased this now after #192 got merged. |
|
@mariushoch Can you move this PR to gerrit? It would be great. |
|
I'm closing this PR as this repo is moved to gerrit (https://gerrit.wikimedia.org/r/#/q/project:mediawiki/extensions/PropertySuggester) and it's WIP and not ready for review (yet). I won't delete this branch (and no one should) to make this easy to pick up and move forward. |
Allow changing the weight given to classifying conditions via the
$wgPropertySuggesterClassifyingConditionWeightsetting. This only applies to the "item" context if there is at least one classifying statement.During testing it turned out that the classifying conditions resulted in way better suggestions than the ones which are purely based on whether a Statement with a given Property id exists. Due to this, I want to be able to boost the weight of these classifying conditions, in order to address T132839.
https://phabricator.wikimedia.org/T132839