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

Add pyani compare #364

Open
wants to merge 73 commits into
base: master
Choose a base branch
from
Open

Add pyani compare #364

wants to merge 73 commits into from

Conversation

baileythegreen
Copy link
Contributor

@baileythegreen baileythegreen commented Dec 13, 2021

Subcommand that performs comparisons between 2+ pyani runs, allowing users to compare methods to each other, or see how altering a parameter affects the results.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality not to work as expected)
  • This change requires a documentation update
  • This is a documentation update

Action Checklist

  • Work on a single issue/concept (if there are multiple separate issues to address, please use a separate pull request for each)
  • Fork the pyani repository under your own account (please allow write access for repository maintainers)
  • Set up an appropriate development environment (please see CONTRIBUTING.md)
  • Create a new branch with a short, descriptive name
  • Work on this branch
    • style guidelines have been followed
    • new code is commented, especially in hard-to-understand areas
    • corresponding changes to documentation have been made
    • tests for the change have been added that demonstrate the fix or feature works
  • Test locally with pytest -v non-passing code will not be merged
  • Rebase against origin/master
  • Check changes with flake8 and black before submission
  • Commit branch
  • Submit pull request, describing the content and intent of the pull request
  • Request a code review
  • Continue the discussion at Pull requests section in the pyani repository

@baileythegreen baileythegreen added the enhancement something we'd like pyani to do that it doesn't already label Dec 13, 2021
@codecov
Copy link

codecov bot commented Dec 13, 2021

Codecov Report

Merging #364 (d01ed41) into master (62949c4) will decrease coverage by 3.23%.
The diff coverage is 28.72%.

@@            Coverage Diff             @@
##           master     #364      +/-   ##
==========================================
- Coverage   75.82%   72.58%   -3.24%     
==========================================
  Files          55       57       +2     
  Lines        3747     4016     +269     
==========================================
+ Hits         2841     2915      +74     
- Misses        906     1101     +195     

@widdowquinn
Copy link
Owner

widdowquinn commented Mar 16, 2022

I've used pyani compare in anger now, and it's awesome. But I do have some notes.

Edited by Bailey to create a checklist.

  • In Bland-Altman plots, we should make the direction of comparison/subtraction explicit in the y-axis label, for instance: "Difference between [values] ([ref_runid] - [comp_runid])"
  • We should have the same kind of explicit label in the titles for the distribution plots, e.g. "Compare [measure] run [ID] - run [ID]"
  • For absolute difference distributions, is it possible to start the x-axis at zero to avoid the implication of negative values with the KDE curve?
  • For heatmaps we also need the explicit direction of subtraction in the title (e.g. "Compare [measure] run [ID] - run [ID]")
  • For absolute difference heatmaps (range [0, inf)) we should not use a divergent colour scheme - as is currently the case. I propose using a perceptually uniform colour map (see, e.g. https://seaborn.pydata.org/tutorial/color_palettes.html).
  • For actual difference heatmaps (range (-inf, inf) we should use a divergent, perceptually uniform palette, centred on zero (see, e.g. https://seaborn.pydata.org/tutorial/color_palettes.html). The current palette diverges, but looks like it centres to the mean
  • I have a general perception that the speed of image generation isn't great - I'm not sure where the bottleneck is. My preference is to make it right, then make it fast, so we should merge this and then look to speed it up (add a new issue)
  • The report looks like a useful start, but we should think about templating it with some general figure interpretation guidance. More important to generate the figures for user interpretation first - so merge first, and then improve the report (add a new issue)

@baileythegreen
Copy link
Contributor Author

baileythegreen commented Apr 5, 2022

@widdowquinn, axis limit, colour map, and title changes have been made based on your suggestions, and pushed. Currently, the distribution plots for signed differences do not have axis limits—though we may want to add them (once we determine what they should be).

Barring any decisions like that, I think this is ready for merging.

@baileythegreen baileythegreen added this to the 0.3.0 milestone May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement something we'd like pyani to do that it doesn't already
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants