[bugfix] fgbio error rate by read position per-base plots

[nh13]

If the error rate for any base-to-base (ex. A>C/a_to_c_error_rate column ) error
was higher than the overall error rate (error_rate column), then it
would be filtered out by y_max.

This fix calculates y_max across not only the overall error rate (across
positions), but also the base-to-base error rate columns.

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

[nh13]

@ewels I was suprised that setting ymax causes data to be discarded versus adjusting the field of view of the data. For example, I would have expected setting ymax to 0.5 would mean not that points like 0.7 would be discarded, just that they'd be off the screen. For a line graph, I would have expected the line to go off the screen, then come back down. I would have also been ok with all values above ymax be set to ymax. But I think the worst thing that can happen is for them to be discarded. Because then in the linegraph, we get zero values for missing data, which falsely tells us the error rate for that position/cycle is zero, when it really was >ymax. Does that make sense why discarding is dangerous?

[nh13]

This is where data is discarded: https://github.com/ewels/MultiQC/blob/fa7e0bf8e7c5f7a6377bd217c13b3cbb12a70b0a/multiqc/plots/linegraph.py#L139

[nh13]

@ewels any thoughts on getting this merged and a new release?

[ewels]

Looks good, thanks!

[ewels]

Apologies @nh13 - I've been swamped with hackathons and conferences lately and am focussing on @nf-core stuff (which has waited longer than MultiQC for a release).

I'll get back to MultiQC + a new release ASAP, but won't be for a few weeks as tomorrow is my last day before some holiday. In the mean time I've merged this so it's at least fixed on dev 👍🏻

[ewels]

ps. Yes I agree that it seems odd and a little dangerous to discard data when above ymax. Digging back through the git blame I apparently added this about 3 years ago (9599630) for the following reason:

• Line graphs and scatter graphs axis limits
  • If limits are specified, data exceeding this is no longer saved in report
  • Visually identical, but can make report file sizes considerable smaller in some cases

If you look in the same commit, I was also modifying the Preseq module - from memory that was spitting out these huge exponential curves with thousands and thousands of points. So in that context it kind of made sense, but I agree that if it's just ditching a single point in an otherwise bumpy line then this looks like missing data and is misleading.

I guess the correct solution here is to only discard those points if the curve doesn't come back under ymax again. Then that can correctly truncate the crazy preseq curves and save on report size, but won't drop data mid-way through a plot. Does that sound ok? I'll make an issue..

Phil

[ewels]

x-ref #1257

[nh13]

Thanks! Enjoy your vacation!

[nh13]

@ewels any chance we could have a release with this included?

[ewels]

@ng13 - the data discarding problem you saw should now be fixed in 74b781e (#1257)

Thanks for spotting it and pointing it out 👍🏻 Let me know if you find any issues with the new code.