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

Seqwho #1503

Merged
merged 14 commits into from Feb 3, 2024
Merged

Seqwho #1503

merged 14 commits into from Feb 3, 2024

Conversation

vsmalladi
Copy link
Contributor

@vsmalladi vsmalladi commented Jul 23, 2021

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md has been updated
  • There is example tool output for tools in the https://github.com/ewels/MultiQC_TestData repository or attached to this PR
  • Code is tested and works locally (including with --lint flag)
  • docs/README.md is updated with link to below
  • docs/modulename.md is created
  • Everything that can be represented with a plot instead of a table is a plot
  • Report sections have a description and help text (with self.add_section)
  • There aren't any huge tables with > 6 columns (explain reasoning if so)
  • Module does not do any significant computational work

Close #1388

@vsmalladi
Copy link
Contributor Author

@ewels any ideas when this can be merged in?

@ewels
Copy link
Member

ewels commented Jan 18, 2022

I'm finally back to working 100% now, so soon hopefully! Hoping to find time to work through all of the open PRs in the next few weeks if all goes well.

@vladsavelyev vladsavelyev added this to the MultiQC v1.20 milestone Dec 15, 2023
@vladsavelyev vladsavelyev changed the title First implementation of seqwho graphs and data. Seqwho Dec 15, 2023
@vladsavelyev vladsavelyev added the awaits-review Awaiting final review and merge. label Jan 24, 2024
Copy link
Member

@vladsavelyev vladsavelyev left a comment

Choose a reason for hiding this comment

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

Merged with main, cleaned up, fixed the lining, and updated the column scales - the code looks good now! Thanks a lot for the contribution, @vsmalladi.

Just one question about the search pattern.

@vladsavelyev vladsavelyev self-requested a review January 26, 2024 12:06
@vsmalladi
Copy link
Contributor Author

Thanks for getting this in

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.

LGTM - the only bit I don't like is the "stacking": None,. This means the plots scale badly, as if you have 100 samples there are now 400 bars which gets unreadable very fast. However, I understand that it's wrong to stack percentages, so I'm not sure I have a better solution.

We've had similar situations before and talked about using a Parallel Coordinates plot. This isn't supported by MultiQC currently (beyond a hacky version with a line plot), but we may be able to add it in the future with Plotly now. That could be something to think about if we want to come back to this in the future.

No better solution that I can think of for now, so happy to merge as-is. Thanks @vsmalladi !

@ewels ewels merged commit c2de371 into MultiQC:main Feb 3, 2024
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. module: new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

seqwho
3 participants