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

Improve performance of .lvCompare() #425

Merged
merged 15 commits into from
Nov 4, 2024

Conversation

Qile0317
Copy link
Collaborator

@Qile0317 Qile0317 commented Oct 23, 2024

A general improvement on the runtime of combineBCR() by accelerating .lvCompare() with rcpp by a small constant factor.

@Qile0317 Qile0317 marked this pull request as ready for review October 23, 2024 01:22
@Qile0317
Copy link
Collaborator Author

@ncborcherding Just realized that the vignette build failed in the R-CMD-check with the following:

Error: Error: processing vignette 'vignette.Rmd' failed with diagnostics:
object 'uni_IG' not found
--- failed re-building ‘vignette.Rmd’

and also

The magick package is required to crop "/tmp/Rtmp7YAETR/Rbuild179452a8e5c7/scRepertoire/vignettes/vignette_files/figure-html/unnamed-chunk-55-1.png" but not available.
Quitting from lines 1146-1159 [unnamed-chunk-57] (vignette.Rmd)

Will get on that later. Unsure why vignette is failing since it wasn't really touched.

@ncborcherding
Copy link
Owner

Looks like the clonalCluster() is broken or at least the internal .lvCompare() based on the error message.

I think the magick warning is a BiocStyle issue.

@Qile0317 Qile0317 marked this pull request as draft October 24, 2024 22:44
@Qile0317 Qile0317 changed the title Improve performance of .lvCompare() Improve performance of .lvCompare() Oct 29, 2024
@Qile0317 Qile0317 self-assigned this Oct 31, 2024
@Qile0317 Qile0317 added enhancement New feature or request help wanted Extra attention is needed labels Oct 31, 2024
})
cluster <- bind_rows(cluster.list)
cluster <- bind_rows(cluster.list) # the TRA_cluster isnt assigned in the failing test
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ncborcherding Could you maybe describe what exactly this intermediate variable is supposed to be? Struggling to debug downstream atm.

Copy link
Owner

Choose a reason for hiding this comment

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

@Qile0317 Here the cluster.list is the cluster assignments from the .lv.compare() function that are grouped into list elements, bind_rows() is just forming a data frame from the list.

@ncborcherding ncborcherding marked this pull request as ready for review November 4, 2024 14:54
@ncborcherding ncborcherding merged commit 223217e into ncborcherding:dev Nov 4, 2024
1 check passed
@Qile0317
Copy link
Collaborator Author

Qile0317 commented Nov 4, 2024

@ncborcherding actually - I just commented out a failing test - I think this is still broken :P this was my fault for not communicating well - I can do another PR for undoing the merge?

@ncborcherding
Copy link
Owner

@Qile0317 Don't worry about it and sorry for not catching it - why don't you make a new PR for the sole issue itself. I think that will save both of use time and be more straightforward.

Nick

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants