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

New module: GoPeaks #1562

Merged
merged 21 commits into from
Dec 31, 2022
Merged

New module: GoPeaks #1562

merged 21 commits into from
Dec 31, 2022

Conversation

gartician
Copy link
Contributor

@gartician gartician commented Oct 11, 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)
  • Each table column has a different colour scale to its neighbour, which relates to the data (eg. if high numbers are bad, they're red)
  • Module does not do any significant computational work

Hi @ewels again, I'm restarting the PR #1521 initiated by @jakevc.
This MultiQC module is for GoPeaks, a new program to call peaks for CUT&TAG and CUT&RUN datasets. It basically parses a JSON log file to report number of peaks per sample via general stats table and a bar graph.
GoPeaks documentation can be found here, though the manuscript is currently under revision..

@gartician
Copy link
Contributor Author

gartician commented Oct 11, 2021

I fixed the input file path error for GoPeaks and it is functioning correctly.

A lot of the errors (e.g. CSP checks) in this PR reflects the errors in the master branch 730ac16 where it's failing in the Linux test. These problems also come up frequently in the September to October 2021 PRs (#1541 #1552 #1553 #1562).

Feel free to get to this when you can! Your time is probably best spent on the paternity leave 🍼

@ewels
Copy link
Member

ewels commented Nov 15, 2021

CSP checks fixed upstream - I've pulled in the changes so that CI test is working now. However, I also did some other additions upstream so now the tests are failing because you don't have the (new) DOI attribute in your module. Docs: https://multiqc.info/docs/#multiqcmodule-class

@ewels
Copy link
Member

ewels commented Nov 15, 2021

So what's going on with these two PRs sorry? You're restarting #1521, does that mean that @jakevc wants me to close that PR in favour of this one?

I'm a little confused that this PR doesn't have any commits from #1521 so it's not a continuation of that work as such... Did you start by copying the raw files from that PR or something @gartician?

@gartician
Copy link
Contributor Author

gartician commented May 13, 2022

Hi @ewels I'm back, I provided a DOI since the paper is in bioRxiv and currently under peer review. It's safe to say #1521 should be deleted since jakevc moved institutes. And yes I started this work by copying the raw files from that PR.

UPDATE: The paper has been accepted for publication yesterday, so it will be out very soon!

@gartician
Copy link
Contributor Author

@ewels can you review the changes to add the GoPeaks module?

@ewels ewels mentioned this pull request May 28, 2022
11 tasks
@ewels
Copy link
Member

ewels commented May 28, 2022

It's safe to say #1521 should be deleted since jakevc moved institutes.

Ok, I've closed #1521 in favour of this PR then 👍🏻

And yes I started this work by copying the raw files from that PR.

For next time - it's better if you don't do this as it loses original attribution for the work. You can fork any public GitHub repository (in this case, a fork of a fork) and continue pull-requests where the person has left or is not responding etc, which is better practice as then those original commits would end up in this PR. Doesn't matter for now though, just so you know if it happens again :)

UPDATE: The paper has been accepted for publication yesterday, so it will be out very soon!

Congratulations! Ok so I'm guessing this PR is suddenly quite high priority for you if the tool is about to get a wave of publicity. I've labelled it as such and will get to it as soon as I can.

I've not been very active lately with MultiQC due to changing jobs, but things are settling slightly and I'm hoping to try to make some headway into the backlog soon. Try to keep a close eye on this PR if you can as reviews nearly always take a bit of back-and-forth, so if you're quick to respond to my review questions then we can get it merged quickly 👍🏻 Hopefully I'll be able to have a stab at it next week.

Phil

@gartician gartician closed this Jun 7, 2022
@ewels
Copy link
Member

ewels commented Jun 7, 2022

Hi @gartician !

What happened here? Was closing this PR intentional?

Phil

@gartician
Copy link
Contributor Author

Sorry I think I accidentally deleted the PR when I tried to solve #1654 at the same time. I will re-open it again

@gartician gartician reopened this Jun 9, 2022
@gartician
Copy link
Contributor Author

The PR should be ready to review again, sorry for the rookie mistake!

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.

Made a few tweaks, hope that's ok - shouldn't be any effect on the report output though.

Looking good, thanks @gartician!

@ewels ewels merged commit 0a7ab93 into MultiQC:master Dec 31, 2022
@gartician
Copy link
Contributor Author

As always, I appreciate the help and suggestions @ewels. Happy new year to you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants