Add controversial ranking #3205

iByteABit256 · 2023-06-19T22:45:17Z

Closes #2515
Added controversy rank property to posts and comments, and ability to sort by it.

Out of the ones previously discussed, I used the controversy_rank_ratio method, but this is still open for debate.
Any input is welcome.

lionirdeadman · 2023-06-20T07:20:45Z

I can't read this code but this should be disabled on clients (web and api) if the instance doesn't allow downvotes as this ranking method becomes useless.

Is this taken into account like the issue mentions?

iByteABit256 · 2023-06-20T07:32:54Z

My thinking is that this should be done on the front end if possible, I've drafted a PR but I haven't written the hiding logic yet.
I think it would become unnecessarily messy overall if done on the back end.

The front end is going to do that job any way, so it doesn't really matter if the controversy rank exists or not in the API responses, does it?

MaartenStaa · 2023-06-20T11:43:15Z

migrations/2023-06-16-220750_create_controversial_indexes/up.sql

+    if downvotes <= 0 or upvotes <= 0 then
+        return 0;
+    else
+        return floor((upvotes + downvotes) * case when upvotes > downvotes then downvotes::float / upvotes::float else upvotes::float / downvotes::float end);
+    end if;


One thing that came up in #2515 was what the sorting algorithm looks like. The algorithm used by Reddit (in 2015) looks to me like it would work better. Comparison here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=400c37593a339dcdb53851e685ecaf75

Note that I changed this from the one in the issue to use a float for the Reddit one, which I think works better for controversy.

I updated the rest of them as well. I think as long as there is a maximum diagonal where values grow according to how close they are to the diagonal, then it's good enough. The Reddit one is quite larger in the diagonal since it raises the activity to the power of the ratio instead of multiplying, so it might avoid some incorrect sorts in close situations, but both are fine I think. My thinking was that since we're going to be running this function pretty heavily, we might as well make it as performant as possible, and powers are pretty bad in that sense.

MaartenStaa · 2023-06-20T11:44:50Z

crates/db_views/src/comment_view.rs

@@ -347,6 +347,9 @@ impl<'a> CommentQuery<'a> {

    query = match self.sort.unwrap_or(CommentSortType::Hot) {
      CommentSortType::Hot => query.then_order_by(comment_aggregates::hot_rank.desc()),
+      CommentSortType::Controversial => {
+        query.then_order_by(comment_aggregates::controversy_rank.desc())


If we don't do the Reddit method of controversy calculation, should this add a second ORDER BY for activity as @dcormier suggested? Same applies for the other places that do this ordering.

MaartenStaa · 2023-06-20T11:45:47Z

crates/db_views_actor/src/person_view.rs

@@ -100,6 +100,7 @@ impl<'a> PersonQuery<'a> {
      SortType::Hot | SortType::Active | SortType::TopAll => {
        query.order_by(person_aggregates::comment_score.desc())
      }
+      SortType::Controversial => query.order_by(person_aggregates::comment_score.asc()),


This doesn't look right to me, should we maintain a person_aggregates::controversy_score?

In what sense is a person controversial though? I find that idea pretty controversial itself :P

I just added that as a dumb fallback since there probably won't be such an option ever implemented in the UI, I could also put it along with SortType::Hot even if it's quite a different concept.

Ah right, so this was added because the same SortType is used here both for posts and for persons, that seems less than ideal. You could add it to the match pattern above together with hot, active, and top all, which also don't really apply to a person.

Might be better though to split that out, so that there's a PostSortType and PersonSortType, alongside the existing CommentSortType. That's a bigger change though 🙂 WDYT?

I agree it would make more sense to add a separate sort type for person, and then also get rid of the unused options (newcomments, hot, active)

Yeah that would be quite better

I made PersonSortType like this:

pub enum PersonSortType { New, Old, MostComments, TopDay, TopWeek, TopMonth, TopYear, TopAll, }

Do we need the same with CommunitySortType? If so, what values should be removed from it?

dessalines

The main issue with this PR, is that

dessalines · 2023-06-20T12:19:09Z

src/scheduled_tasks.rs

  scheduler.every(CTimeUnits::minutes(5)).run(move || {
    update_hot_ranks(&mut conn_2, true);
+    update_controversy_ranks(&mut conn_2, true);


Here's the main issue with this PR, that should get you to rethink it. The only reason hot ranks need to be recalculated, is because its a decaying time function. I forget the term in SQL, but its essentially based on an unstable function.

A controversial sort, which works only on upvotes, downvotes, and score, is not that. It can be done entirely with vote triggers.

Just like the post_aggregates.score column, you need to update the vote triggers to run whatever logic necessary to update that controversial_rank column.

Nutomic · 2023-06-20T14:20:22Z

I can't read this code but this should be disabled on clients (web and api) if the instance doesn't allow downvotes as this ranking method becomes useless.

If votes are disabled, that comes back with LocalSite, and clients can hide this sort option accordingly.

iByteABit256 · 2023-06-23T17:54:15Z

Marking as draft for now, will open it again once I divide the Sort types and do some more testing

Nutomic · 2023-06-29T09:17:25Z

crates/db_schema/src/utils.rs

+    SortType::TopYear =>  PersonSortType::TopYear,
+    SortType::TopMonth => PersonSortType::TopMonth,
+  }
+}


Bit awkward that this is needed, but I guess there is no other way to make search all work.

Do you think the same should be done with CommunitySortType or is it too much?

It probably should, but you don't need to do that in this PR unless you want to. A lot of the Post sorts don't make sense for communities either.

Nutomic · 2023-06-29T09:21:43Z

crates/db_schema/src/newtypes.rs

@@ -35,7 +35,7 @@ impl fmt::Display for PostId {
 /// The person id.
 pub struct PersonId(pub i32);

-#[derive(Debug, Copy, Clone, Hash, Eq, PartialEq, Serialize, Deserialize, Default)]
+#[derive(Debug, Copy, Clone, Hash, Eq, PartialEq, PartialOrd, Serialize, Deserialize, Default)]


PartialOrd seems to be unused

… sort by it

…s don't seem to get updated with floats, divide SortTypes

migrations/2023-06-16-220750_create_controversial_indexes/up.sql

crates/db_views_actor/src/person_view.rs

crates/db_schema/src/lib.rs

dessalines · 2023-07-03T15:48:23Z

crates/db_schema/src/lib.rs

+  New,
+  Old,
+  MostComments,
+  TopDay,


See comment above, but yeah I think you can get rid of all these Top sorts, and just add CommentScore

In the post_to_person_sort_type util, does it make sense to use CommentScore as a default so there aren't so many mappings because of the SortType's Top sorts?

pub fn post_to_person_sort_type(sort: SortType) -> PersonSortType { match sort { SortType::Active | SortType::Hot | SortType::Controversial => PersonSortType::CommentScore, SortType::New | SortType::NewComments => PersonSortType::New, SortType::MostComments => PersonSortType::MostComments, SortType::Old => PersonSortType::Old, _ => PersonSortType::CommentScore, } }

dessalines · 2023-07-03T15:49:32Z

crates/db_schema/src/utils.rs

+    SortType::TopYear =>  PersonSortType::TopYear,
+    SortType::TopMonth => PersonSortType::TopMonth,
+  }
+}


It probably should, but you don't need to do that in this PR unless you want to. A lot of the Post sorts don't make sense for communities either.

…comments

…SortType and added CommentScore instead

…comments

…com:iByteABit256/lemmy into lemmy-2515-controversial-posts-and-comments

iByteABit256 · 2023-07-21T16:08:58Z

@dessalines I've made the changes you requested in case you hadn't noticed

migrations/2023-06-16-220750_create_controversial_indexes/up.sql

dessalines · 2023-07-25T20:03:08Z

My bad for being so slow to get to this, I apologize.

iByteABit256 · 2023-07-25T20:13:25Z

No worries 😅

…comments

…ing controversy rank

dessalines

Perfect, thanks!

* Added controversy rank property to posts and comments, and ability to sort by it * Triggers instead of schedules tasks, integer -> double, TODO: comments don't seem to get updated with floats, divide SortTypes * Created PersonSortType * PersonSortType::MostComments case * Removed unused PartialOrd trait * Added new person sort type mappings * SortType -> PersonSortType * fixes * cargo fmt * fixes after merge with main * Fixed bug in controversy rank trigger, removed TopX sorts from PersonSortType and added CommentScore instead * Uncovered enum case * clippy * reset translation changes * translations * translations * Added additional hot ordering on controversial posts and comments * featured local and featured community added to controversy rank index, additional order_by removed (?), added post_score and post_count to PersonSortType * Woodpecker rerun * cargo fmt * woodpecker rerun * fixed controversy_rank order * fix * Readded migration as latest, removed second update statement for setting controversy rank

This reverts commit c5450e7.

phiresky · 2023-07-30T13:05:55Z

I guess I'm kinda late with this.. but since controversy_rank seems purely computed from other columns in the same table, it looks like this could be done with a GENERATED ALWAYS AS controversial_rank(upvotes, downvotes) column which would have avoided the trigger changes and the expensive migration

iByteABit256 · 2023-07-30T16:21:58Z

Didn't know about GENERATED ALWAYS AS, it would have been quite better indeed :/

It would store less data that way too

The good thing is that if the votes don't change that often for a large amount of posts, then those calculations will be avoided using triggers

iByteABit256 requested review from Nutomic and dessalines as code owners June 19, 2023 22:45

MaartenStaa reviewed Jun 20, 2023

View reviewed changes

dessalines reviewed Jun 20, 2023

View reviewed changes

Nutomic changed the title ~~Closes #2515~~ Add controversial ranking Jun 22, 2023

iByteABit256 marked this pull request as draft June 23, 2023 17:53

iByteABit256 mentioned this pull request Jun 26, 2023

Closes #1404 LemmyNet/lemmy-ui#1405

Closed

Nutomic reviewed Jun 29, 2023

View reviewed changes

iByteABit256 marked this pull request as ready for review June 30, 2023 23:35

iByteABit256 requested review from Nutomic and dessalines July 2, 2023 15:11

iByteABit256 and others added 10 commits July 3, 2023 17:47

Added controversy rank property to posts and comments, and ability to…

cccc538

… sort by it

Triggers instead of schedules tasks, integer -> double, TODO: comment…

a5defb4

…s don't seem to get updated with floats, divide SortTypes

Created PersonSortType

3723100

PersonSortType::MostComments case

683d360

Removed unused PartialOrd trait

6d65d78

Added new person sort type mappings

968c528

SortType -> PersonSortType

c82ec69

fixes

3aa5ee1

cargo fmt

1498e95

fixes after merge with main

11579d1

dessalines reviewed Jul 3, 2023

View reviewed changes

iByteABit256 force-pushed the lemmy-2515-controversial-posts-and-comments branch from c3a7270 to 11579d1 Compare July 3, 2023 15:55

iByteABit256 and others added 2 commits July 3, 2023 22:21

Merge branch 'LemmyNet:main' into lemmy-2515-controversial-posts-and-…

bf69785

…comments

Fixed bug in controversy rank trigger, removed TopX sorts from Person…

72a4b50

…SortType and added CommentScore instead

iByteABit256 requested review from dessalines and Nutomic July 14, 2023 16:56

Merge branch 'main' into lemmy-2515-controversial-posts-and-comments

8276cc2

iByteABit256 mentioned this pull request Jul 19, 2023

Lemmy 2515 controversial posts and comments LemmyNet/lemmy-ui#1727

Merged

Merge branch 'LemmyNet:main' into lemmy-2515-controversial-posts-and-…

f904cd2

…comments

iByteABit256 mentioned this pull request Jul 20, 2023

Controversial translations - English and Greek LemmyNet/lemmy-translations#90

Merged

iByteABit256 and others added 9 commits July 20, 2023 20:42

Merge branch 'main' into lemmy-2515-controversial-posts-and-comments

61152b8

Woodpecker rerun

0e6d022

Merge branch 'main' into lemmy-2515-controversial-posts-and-comments

35bff32

cargo fmt

9bd97b9

woodpecker rerun

b38c0c9

Merge branch 'main' into lemmy-2515-controversial-posts-and-comments

6d11656

fixed controversy_rank order

ba0c253

Merge branch 'lemmy-2515-controversial-posts-and-comments' of github.…

cb2d3ac

…com:iByteABit256/lemmy into lemmy-2515-controversial-posts-and-comments

fix

a5a9124

dessalines reviewed Jul 25, 2023

View reviewed changes

iByteABit256 and others added 2 commits July 26, 2023 02:44

Merge branch 'LemmyNet:main' into lemmy-2515-controversial-posts-and-…

1c50d90

…comments

Readded migration as latest, removed second update statement for sett…

fc455d4

…ing controversy rank

iByteABit256 requested a review from dessalines July 26, 2023 00:36

dessalines approved these changes Jul 26, 2023

View reviewed changes

dessalines merged commit c890797 into LemmyNet:main Jul 26, 2023
1 check passed

dessalines added a commit that referenced this pull request Jul 27, 2023

Revert "Add controversial ranking (#3205)"

6bc86ba

This reverts commit c5450e7.

iByteABit256 mentioned this pull request Jul 27, 2023

Controversial sorting for posts and comments LemmyNet/jerboa#1105

Closed

3 tasks

MV-GH mentioned this pull request Oct 14, 2023

Add a "Controversial" sort option #4043

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add controversial ranking #3205

Add controversial ranking #3205

iByteABit256 commented Jun 19, 2023 •

edited

lionirdeadman commented Jun 20, 2023

iByteABit256 commented Jun 20, 2023 •

edited

MaartenStaa Jun 20, 2023

iByteABit256 Jun 21, 2023 •

edited

MaartenStaa Jun 20, 2023

MaartenStaa Jun 20, 2023

iByteABit256 Jun 21, 2023 •

edited

MaartenStaa Jun 23, 2023

Nutomic Jun 23, 2023

iByteABit256 Jun 23, 2023

iByteABit256 Jun 28, 2023

dessalines left a comment

dessalines Jun 20, 2023

Nutomic commented Jun 20, 2023

iByteABit256 commented Jun 23, 2023

Nutomic Jun 29, 2023

iByteABit256 Jun 29, 2023

dessalines Jul 3, 2023

Nutomic Jun 29, 2023

dessalines Jul 3, 2023

iByteABit256 Jul 3, 2023

dessalines Jul 3, 2023

iByteABit256 commented Jul 21, 2023

dessalines commented Jul 25, 2023

iByteABit256 commented Jul 25, 2023

dessalines left a comment

phiresky commented Jul 30, 2023

iByteABit256 commented Jul 30, 2023 •

edited

Add controversial ranking #3205

Add controversial ranking #3205

Conversation

iByteABit256 commented Jun 19, 2023 • edited

lionirdeadman commented Jun 20, 2023

iByteABit256 commented Jun 20, 2023 • edited

Choose a reason for hiding this comment

iByteABit256 Jun 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iByteABit256 Jun 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic commented Jun 20, 2023

iByteABit256 commented Jun 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iByteABit256 commented Jul 21, 2023

dessalines commented Jul 25, 2023

iByteABit256 commented Jul 25, 2023

dessalines left a comment

Choose a reason for hiding this comment

phiresky commented Jul 30, 2023

iByteABit256 commented Jul 30, 2023 • edited

iByteABit256 commented Jun 19, 2023 •

edited

iByteABit256 commented Jun 20, 2023 •

edited

iByteABit256 Jun 21, 2023 •

edited

iByteABit256 Jun 21, 2023 •

edited

iByteABit256 commented Jul 30, 2023 •

edited