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

Port over quietT/BCRgenes() #459

Merged
merged 20 commits into from
Jan 8, 2025
Merged

Port over quietT/BCRgenes() #459

merged 20 commits into from
Jan 8, 2025

Conversation

Qile0317
Copy link
Collaborator

@Qile0317 Qile0317 commented Dec 25, 2024

copies over the quietTCRgenes() and quietBCRgenes() functions from Trex and Ibex, and also adds a function quietVDJgenes() that runs both as a convenience. Lastly, some small changes are that all instances of the github checkout action has been upgraded to v4 from v3, and some redundant imports in DESCRIPTION have been trimmed.

@Qile0317 Qile0317 added the enhancement New feature or request label Dec 25, 2024
@Qile0317 Qile0317 self-assigned this Dec 25, 2024
@Qile0317 Qile0317 changed the title Port over quietT/BCRgenes Port over quietT/BCRgenes and use checkout@v4 Dec 26, 2024
@Qile0317
Copy link
Collaborator Author

@ncborcherding In the future when this version goes into the main bioconductor server, the sourcecode for those functions in Ibex and Trex can be removed and they can re-export those functions instead.

@Qile0317 Qile0317 changed the title Port over quietT/BCRgenes and use checkout@v4 Port over quietT/BCRgenes() and use checkout@v4 Dec 26, 2024
@Qile0317 Qile0317 changed the title Port over quietT/BCRgenes() and use checkout@v4 Port over quietT/BCRgenes() and add findVariableNonVdjFeatures() Dec 26, 2024
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 Ensure the new changelog info I added are in the correct location in NEWS.md?

@ncborcherding
Copy link
Member

@Qile0317 I don't think we can have functions that depend on Seurat - it is a massive dependency hole. I would prefer keeping everything to SeuratObject. Maybe we can keep the quiet functions and not have an independent variable gene function?

@Qile0317
Copy link
Collaborator Author

Qile0317 commented Dec 31, 2024

@Qile0317 I don't think we can have functions that depend on Seurat - it is a massive dependency hole. I would prefer keeping everything to SeuratObject. Maybe we can keep the quiet functions and not have an independent variable gene function?

@ncborcherding Right - so in this configuration Seurat is actually in Suggests and nothing from Seurat is actually loaded or imported. Rather the namespace is required to be loaded at runtime instead of package build time. Everything that was prefixed with Seurat:: is lazy loaded here and the variable gene filtering method would never have dispatched to the Seurat S3 method if a seurat object made from a presumably loaded Seurat package is already loaded. But I also understand the concern about Seurat as a "soft" dependency, so let me know if you'd still prefer to not have it and I'll remove it.

@ncborcherding
Copy link
Member

@Qile0317

Think we need to drop it as that is not in keeping with Bioconductor policy and they also want equal support for SingleCellExperiment. Lets just do the quiet() functions.

Thanks,
Nick

@Qile0317
Copy link
Collaborator Author

Ok, sounds good, will remove now.

@Qile0317 Qile0317 changed the title Port over quietT/BCRgenes() and add findVariableNonVdjFeatures() Port over quietT/BCRgenes() Dec 31, 2024
@Qile0317 Qile0317 requested review from ncborcherding and removed request for ncborcherding January 6, 2025 10:22
@ncborcherding ncborcherding merged commit ebccc86 into BorchLab:dev Jan 8, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants