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

Differing dendrogram outputs from plot - --labeltext is honored differently by dendrogram and matrix outputs #2667

Closed
Glfrey opened this issue Jul 5, 2023 · 6 comments · Fixed by #2790
Assignees

Comments

@Glfrey
Copy link

Glfrey commented Jul 5, 2023

Hello,

I'm currently having an issue where sourmash plot is outputting two different dendrograms. I've attached an example and the code I used below. I get the same issue with version 4.0.0.(using for some old work) and the latest version.

sourmash compare -k=21 --output k21_all_compare --csv k21_all_compare.csv /home/b56j439/sourmash/all/sigs/*sig

sourmash plot k21_all_compare --labeltext new_labels.txt --csv k21_all_plot.csv

I've confirmed the new_labels.txt contains the same labels in the same order as the original label file, I just wanted to remove the directory structure from the labels.

k21_all_compare matrix
k21_all_compare dendro

@mr-eyes
Copy link
Member

mr-eyes commented Jul 5, 2023

Hi,
To make sure I understand the issue. In summary, the only difference here is the tree coloring in the clustermap and the dendrogram, right?

Update: I can see the problem now. Thanks.

@Glfrey
Copy link
Author

Glfrey commented Jul 6, 2023

Thanks @mr-eyes , let me know if you need anything from me.

@mr-eyes mr-eyes self-assigned this Jul 8, 2023
This was referenced Jul 9, 2023
@Glfrey
Copy link
Author

Glfrey commented Jul 9, 2023

Hi @mr-eyes,

I have identified the problem, part of which was my fault ( I was missing the -labels argument) and the other part was due to asymmetric handling of the -labels argument for the two plots. I've implemented a fix and opened a pull request.

@mr-eyes
Copy link
Member

mr-eyes commented Jul 9, 2023

Thanks @Glfrey
I will take a deeper look in a few days and update the issue.

@ctb ctb changed the title Differing dendrogram outputs Differing dendrogram outputs from plot - --labeltext is honored differently by dendrogram and matrix outputs Jul 11, 2023
@ctb
Copy link
Contributor

ctb commented Jul 11, 2023

ok, got it ;).

The problem is that the matrix plot is not observing the labels passed in by --labeltext without --labels, while the dendrogram plot is observing those labels.

One fix (that would not be backwards compatible necessarily) is to force args.labels to True if --labeltext is provided.

Related, it seems to me that having sourmash_fig.plot_composite_matrix take both show_labels and show_indices is confusing...

@ctb
Copy link
Contributor

ctb commented Sep 28, 2023

To test this all out, examine the dendrogram and matrix output of the following commands:

  1. sourmash plot compare-demo - labels on dendrogram, indices on matrix (BUG)

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

  3. sourmash plot compare-demo --indices - labels on dendrogram, no indices on matrix (BUG)

  4. sourmash plot compare-demo --labels --indices - labels on both ✅

#2790 fixes this behavior.

@ctb ctb closed this as completed in #2790 Oct 15, 2023
ctb added a commit that referenced this issue Oct 15, 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:

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

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

7. `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:
- [x] write some tests for new args
- [x] update documentation
- [x] check to see if notebook code should be updated

Fixes #2667
Closes #2672
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants