Skip to content
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

Broken "sort groups by..." for multiple sort criteria #161

Closed
thinkh opened this issue Jun 11, 2019 · 4 comments · Fixed by #273
Closed

Broken "sort groups by..." for multiple sort criteria #161

thinkh opened this issue Jun 11, 2019 · 4 comments · Fixed by #273
Assignees
Labels
lineup: v4 All issues related to LineUp v4 type: bug Something isn't working
Projects

Comments

@thinkh
Copy link
Member

thinkh commented Jun 11, 2019

  • Release number or git hash: 169bd48
  • Web browser version and OS: Chrome 75

Steps to reproduce

I could not provide a sandbox link, as a sorting bug was fixed with bafe482 (after v4.0.0-alpha.11) and no alpha.12 is available (yet).

  1. Run npm start and open http://localhost:8080/aggregate.html
  2. Column cat -> column menu -> sort groups by... -> descending
  3. Column cat2 -> column menu -> sort groups by... -> descending/ascending

Observed behavior

lineup-sort-groups

Only the first group sorting is applied (in the example cat) and the second group sorting has no effect.

Expected behavior

  • All group sort criteria should be considered
@thinkh thinkh added the type: bug Something isn't working label Jun 11, 2019
@sgratzl sgratzl added this to To do in lineup Jun 15, 2019
@sgratzl sgratzl moved this from To do to Needs review in lineup Jun 17, 2019
@sgratzl sgratzl moved this from Needs review to Done in lineup Jun 17, 2019
@thinkh thinkh added the lineup: v4 All issues related to LineUp v4 label Jul 12, 2019
@sgratzl sgratzl self-assigned this Feb 19, 2020
@sgratzl
Copy link
Member

sgratzl commented Feb 19, 2020

so far when group sorting by a categorical value it actually sorts by two criteria: the name of the most frequent group, and second its count.

export function toGroupCompareCategoryValue(rows: ISequence<IDataRow>, col: ICategoricalColumn, valueCache?: ISequence<ICategory | null>): ICompareValue[] {
if (isSeqEmpty(rows)) {
return [NaN, 0];
}
const mostFrequent = findMostFrequent(rows.map((d) => col.getCategory(d)), valueCache);
if (mostFrequent.cat == null) {
return [NaN, 0];
}
return [mostFrequent.cat.value, mostFrequent.count];
}

e.g. in builder3.html

image

first criteria level: Cat, 2nd. Cat2

values by group:

  1. c1,c2:
  2. most frequent c1, count 8
  3. most frequent c2, count 8
  4. c1,c3:
  5. most frequent c1, count 8
  6. most frequent c2, count 8
  7. c1, c1
  8. most frequent c1, 12
  9. most frequent c1, 12

sorting asc it means: c1,c2 < c1,c1 since the group since of the first group sort criteria is smaller.

@thinkh
Copy link
Member Author

thinkh commented Mar 23, 2020

Thanks for the explanation.

What does most frequent group mean in this case? Is it the size (number of items) of a group?

I think for the user it's very opaque what the group sorting criteria is. In my opinion we have the following options:

  1. Sort groups by category name only: Pretty obvious, since it works the same as the item sorting hierarchy and the category name is what the user can see
    lineup-sort-items
  2. Sort groups by most frequent group: Only visible to the user if the group name column is available. It needs to better communicated/displayed to the user.
  3. User choice: Alternatively, we can also leave the choice to the user and add another radio button to the sort groups by dialog with the options category title (default) and category size. It could look similar to the group by dialog of a string column:
    grafik

@sgratzl What do you think?

@thinkh
Copy link
Member Author

thinkh commented Mar 23, 2020

After checking issue #109 it seems like these two issues are pretty similar.

@sgratzl You posted already a screenshot #109 (comment) which matches my feature request in the previous comment.

Maybe we move the discussion to issue #109 and continue there. I still owe you a comment there. 😃

@sgratzl
Copy link
Member

sgratzl commented Mar 25, 2020

In the end a simple compromise is to sort groups of categoricals by their most frequent category name only. Thus, in case it is used as a grouping criteria it behaves as expected when combined with another group sorting criteria

@sgratzl sgratzl linked a pull request Mar 25, 2020 that will close this issue
4 tasks
@thinkh thinkh closed this as completed Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lineup: v4 All issues related to LineUp v4 type: bug Something isn't working
Projects
No open projects
lineup
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants