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

Bar graphs: add sort_samples: false config option, also do not sort OrderedDict data #2210

Merged
merged 8 commits into from Dec 12, 2023

Conversation

vladsavelyev
Copy link
Member

@vladsavelyev vladsavelyev commented Dec 4, 2023

Fix an artefact of removing OrderedDicts #2154

The idea of the original code seems this: before Python 3.6, regular dict were randomly shuffled, so MultiQC needed to post-sort the sample list to get some reasonable order. But if the module wrapped data in an OrderedDict, it meant that the user explicitly provided the order, and it's not needed to be sorted further:

if isinstance(d, OrderedDict):
    hc_samples = list(d.keys())
else:
    hc_samples = sorted(list(d.keys()))

After Python 3.6, regular dicts keep order, so the need for that sorting has disappeared. So instead, module devs started to assume that OrderedDict is a hacky way to tell MultiQC to disable auto-sorting. Instead, we should always keep the order of passed dictionaries, and treat OrderedDicts and dicts equally (we wouldn't use OrderedDict anywhere internally anyway).

Fixes #2204

@vladsavelyev vladsavelyev added the awaits-review Awaiting final review and merge. label Dec 4, 2023
@ewels
Copy link
Member

ewels commented Dec 11, 2023

Happy only if:

  • The order of samples is maintained between runs
  • The default of order samples is alphabetical for the majority of modules where sample order is not important

It would be nice to see an example where this PR changes the output behaviour to think about this.

May need a new config option to disable alphabetical sorting? I agree that the previous behaviour with OrderedDicts was a bit hacky.

If basically nothing is changing, then happy to merge 👍🏻

Copy link
Contributor

github-actions bot commented Dec 11, 2023

🚀 Deployed on https://mqc-pr-2210--multiqc.netlify.app

@vladsavelyev
Copy link
Member Author

vladsavelyev commented Dec 11, 2023

Fair points, decided to keep the order of OrderedDict inputs for legacy reasons, and also added an non-hacky way to disable sorting with the option pconfig.sort_samples which is True by default.

The order should be stable between runs.

@vladsavelyev vladsavelyev changed the title Remove sorting of bar graph data Bar graphs: add sort_samples: false config option, also do not sort OrderedDict data Dec 11, 2023
@vladsavelyev vladsavelyev enabled auto-merge (squash) December 11, 2023 15:23
@vladsavelyev vladsavelyev added this to the MultiQC v1.19 milestone Dec 12, 2023
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks!

@vladsavelyev vladsavelyev merged commit 2be5617 into master Dec 12, 2023
7 checks passed
@vladsavelyev vladsavelyev deleted the remove-sorting-bargraph branch December 12, 2023 10:12
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: back end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version 1.18 ignores ordering of an OrderedDict
3 participants