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

Fix output format in triangle mode #31

Merged
merged 2 commits into from
Jun 28, 2024
Merged

Fix output format in triangle mode #31

merged 2 commits into from
Jun 28, 2024

Conversation

fplazaonate
Copy link
Contributor

In triangle mode, when an output file is provided, skani generates:

  1. A lower triangular matrix without a diagonal by default
  2. A lower triangular matrix with a diagonal when --full-matrix is provided

IMO, the correct behaviour should be:

  1. A lower triangular matrix with a diagonal by default
  2. A full matrix when --full-matrix is provided

What do you think?

@bluenote-1577
Copy link
Owner

Hi @fplazaonate,

Good to hear from you.

  1. I'm a bit confused about the --full-matrix condition: in the latest skani v0.2.1, --full-matrix gives a full matrix, not a lower triangular matrix with a diagonal. Let me know if I misunderstood

image

  1. The choice of not including a diagonal is intentional -- mash outputs lower triangular matrices in this form, and I wanted compatibility between the two software. See https://mothur.org/wiki/phylip-formatted_distance_matrix/ I agree that this matrix format is not ideal, though...

Thanks

@fplazaonate
Copy link
Contributor Author

Hi @bluenote-1577 ,

Indeed, skani gives the full matrix when the output file is stdout.
The problem occurs with the '-o' option

@bluenote-1577
Copy link
Owner

bluenote-1577 commented Jun 28, 2024

@fplazaonate ah, I see. that's indeed a bug.

I'm happy to merge your pull request if you could remove the = condition (so lower triangle matrix don't have diagonal); in this case, I don't think you can comment out the if statement

@fplazaonate
Copy link
Contributor Author

It should be fixed now.

@bluenote-1577 bluenote-1577 merged commit 9709a52 into bluenote-1577:main Jun 28, 2024
@bluenote-1577
Copy link
Owner

Thanks!

@fplazaonate
Copy link
Contributor Author

BTW, I don't understand why the code for writing to stdout is different than for a regular file.

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.

2 participants