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

Allow specifying default sort columns for tables #1667

Merged
merged 15 commits into from Nov 21, 2023

Conversation

jchorl
Copy link
Contributor

@jchorl jchorl commented Apr 10, 2022

#1666 upgrades the tablesorter plugin. The new version of tablesorter allows you to specify a default sort order via data- html attribute.

This PR extends that to thread a defaultsort option through per-table configs and actually do the default sorting.

Here is an example config:

custom_plot_config:
  general_stats_table:
    defaultsort:
      - [2,1]
      - [3,0]

This specifies to sort:

  • First by column 2, desc
  • Then by column 3, asc

I am soliciting feedback on the interface. I see two options here:

  1. As-is. User specifies the column indexes, and the sort order. This is nice because it ~exactly conforms to the tablesorter spec - it's a 1:1 passthrough. However, this means re-ordering the columns or adding/removing could change the defaultsort behaviour.
  2. User specifies column names, e.g.
    custom_plot_config:
      general_stats_table:
        defaultsort:
          - "foo-col": asc
          - "bar-col": desc
    the interface here is much more user-friendly. But by the time we go to assemble the table in table.py, it seems we only have the header IDs, and not necessarily the names (they're buried in pre-assembled html content). Therefore I think the implementation of this approach would be quite messy and difficult to maintain.

The first seemed much simpler to me.

Once we get consensus, happy to update the docs!

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md has been updated

[UPD by Vlad on 21 Nov 23]: final implementation below. Col IDs and titles are accepted here.

custom_plot_config:
  general_stats_table:
    defaultsort:
      - column: "Mean Insert Length"
        direction: asc
      - column: "Starting Amount (ng)"
  quast_table:
    defaultsort:
      - column: "Largest contig"

@ewels
Copy link
Member

ewels commented May 9, 2022

Therefore I think the implementation of this approach would be quite messy and difficult to maintain.

Difficult to maintain, how come? To me this seems much easier to maintain for users. Columns could come and go quite easily with MultiQC versions / new tool support / tweaks in analysis etc. Column IDs should be fairly stable.

I think I use column IDs for some other stuff already. I have a feeling that they're printed in the configuration modal in the report front end (where you show and hide columns).

@jchorl
Copy link
Contributor Author

jchorl commented Jun 5, 2022

Therefore I think the implementation of this approach would be quite messy and difficult to maintain.

Difficult to maintain, how come? To me this seems much easier to maintain for users. Columns could come and go quite easily with MultiQC versions / new tool support / tweaks in analysis etc. Column IDs should be fairly stable.

I think I use column IDs for some other stuff already. I have a feeling that they're printed in the configuration modal in the report front end (where you show and hide columns).

Okay, yeah, good call. I got this working with column names. Here's a screenshot:
Screenshot 2022-06-05 at 01-47-27 MultiQC Report

I think it's cleaner, and I updated docs. Let me know what you think!

jchorl added a commit to jchorl/MultiQC that referenced this pull request Sep 8, 2022
jchorl added a commit to jchorl/MultiQC that referenced this pull request Sep 8, 2022
jchorl added a commit to jchorl/MultiQC that referenced this pull request Oct 14, 2022
@vladsavelyev
Copy link
Member

Awesome stuff. Referencing columns by titles is user-friendly, but I'd allow referencing by IDs as well, like it's done by default in other configuration sections like table_columns_visible.

I'll try to do that, and will merge with master as well :)

@ewels
Copy link
Member

ewels commented Nov 21, 2023

Yup, IDs is better than names I think. Just position index that I wasn't keen on.

@vladsavelyev
Copy link
Member

Minor changes - I also allowed to omit the "order" as it defaults to "desc" anyway, and added a docs example of sorting general stats. Looks great!

@vladsavelyev vladsavelyev added the awaits-review Awaiting final review and merge. label Nov 21, 2023
@vladsavelyev vladsavelyev added this to the MultiQC v1.19 milestone Nov 21, 2023
@vladsavelyev vladsavelyev self-requested a review November 21, 2023 14:20
@vladsavelyev vladsavelyev merged commit 05743ad into MultiQC:master Nov 21, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaits-review Awaiting final review and merge. core: front end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants