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

MRG: make sourmash plot labels/indices arguments make sense #2790

Merged
merged 12 commits into from
Oct 15, 2023

Conversation

ctb
Copy link
Contributor

@ctb ctb commented Sep 28, 2023

This PR rationalizes sig plot arguments for --labels (show names) and --indices (show numbers), and adds --no-labels and --no-indices, as follows. See #2667 for motivating bug.

  1. sourmash plot compare-demo - labels on dendrogram, labels on matrix ✅ (FIXED)

  2. sourmash plot compare-demo --labels - labels on both dendrogram and matrix ✅

  3. sourmash plot compare-demo --indices - indices on both dendrogram and matrix ✅

  4. sourmash plot compare-demo --labels --indices - labels on both ✅ (FIXED - labels override indices)

New arguments from this PR:

  1. sourmash plot compare-demo --no-labels - indices on both ✅

  2. sourmash plot compare-demo --no-labels --no-indices - no labels/indices on either ✅

  3. sourmash plot compare-demo --no-indices - labels on both ✅

The PR also simplifies some of the plot command code as well as code in fig.py.

TODO:

  • write some tests for new args
  • update documentation
  • check to see if notebook code should be updated

Fixes #2667
Closes #2672

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #2790 (b90f761) into latest (24bf474) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           latest    #2790   +/-   ##
=======================================
  Coverage   86.26%   86.27%           
=======================================
  Files         130      130           
  Lines       14742    14750    +8     
  Branches     2621     2623    +2     
=======================================
+ Hits        12717    12725    +8     
  Misses       1724     1724           
  Partials      301      301           
Flag Coverage Δ
hypothesis-py 25.81% <0.00%> (-0.02%) ⬇️
python 92.85% <100.00%> (+<0.01%) ⬆️
rust 50.92% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/sourmash/cli/plot.py 100.00% <100.00%> (ø)
src/sourmash/commands.py 91.48% <100.00%> (+0.08%) ⬆️
src/sourmash/fig.py 88.37% <100.00%> (-0.76%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ctb ctb changed the title WIP: make sourmash plot labels/indices arguments make sense MRG: make sourmash plot labels/indices arguments make sense Sep 29, 2023
@ctb
Copy link
Contributor Author

ctb commented Sep 29, 2023

Ready for review @sourmash-bio/devs !

Note that the code coverage annotations in the diff are not correct. Not sure what's going on there - I'm going to try rerunning the tests.

@ctb
Copy link
Contributor Author

ctb commented Sep 29, 2023

well, the code cov report is correct, even if the github diff display is not. 🤷

@AnneliektH
Copy link

Arguments all work correctly for my data:

  • --no-indices shows no indices, but shows labels
  • --no-labels gives no labels, but shows indices
  • --no-labels --no-indices gives neither
  • --labels --indices chooses labels over indices.

The way indices work make sense, but got me confused for a second, as it starts with 0, and i worked with a dataset named Sample_1 - Sample_10. Sample_1 is 0 in the indices, Sample_10 is 1 in the indices. Which makes sense from a computer perspective, but maybe less from a human perspective.

@ctb
Copy link
Contributor Author

ctb commented Oct 15, 2023

thanks @AnneliektH ! b90f761 changes the indices to be 1-based. I'll merge when tests pass here!

@ctb ctb merged commit 9ff523e into latest Oct 15, 2023
35 checks passed
@ctb ctb deleted the refactor_plot_indices branch October 15, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Differing dendrogram outputs from plot - --labeltext is honored differently by dendrogram and matrix outputs
2 participants