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

Vucinick patch #10

Merged
merged 10 commits into from
Aug 31, 2024
Merged

Vucinick patch #10

merged 10 commits into from
Aug 31, 2024

Conversation

vucinick
Copy link
Contributor

Hey guys,

While using scdrake, there were a few aesthetic and technical problems I encountered. Here are my (simple) fixes. :)

Short summary of introduced changes:

  1. Fixed deprecated commands in test-run.R
  2. Fixed header in test-cli.R
  3. Fixed a bug in plotting clustree when best K is one of the selected K values
  4. Fixed cluster number ordering in the report

vucinick and others added 5 commits July 12, 2024 09:33
Please check if this is correct.
`run_single_sample()` and `run_integration()` are soft-deprecated since scdrake 1.4.0.  Using `run_single_sample_r()` and `run_integration_r()` instead.
Clusters were ordered in the following manner 1, 10, 11, 12, 13 .. etc. Changed to 1, 2, 3, 4 etc.
If numbered clusters, order by numbers. Otherwise, keep it as is.
Bug noticed by Lucie Pfeiferova. Error when tryint to create a tibble:  Column name `k1` must not be duplicated. Tibble wouldn't have unique column names because best would be a part of the k values. Fixed by taking only unique values.
@vucinick vucinick requested review from gorgitko and jakubovciak July 12, 2024 09:49
Copy link
Contributor

@gorgitko gorgitko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job @vucinick! I left a small comment, maybe just my misunderstanding.

You can also remove these forgotten commented lines in /~https://github.com/bioinfocz/scdrake/pull/10/files#diff-80094c1be9601ba6522be922f9abc07e36cf96a340be7529976f810f8e6a63c7R590-R594

EDIT: I've fixed the test runner script and CI now fails correctly, see /~https://github.com/bioinfocz/scdrake/actions/runs/9942667887/job/27464672026?pr=10#step:12:1001

Could you fix it, please? 🙏

@gorgitko gorgitko self-requested a review August 31, 2024 14:53
@gorgitko gorgitko merged commit d859db4 into main Aug 31, 2024
1 check passed
@gorgitko gorgitko deleted the vucinick-patch-1 branch August 31, 2024 15:10
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