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 conversion fuction #158

Open
wants to merge 43 commits into
base: master
Choose a base branch
from
Open

Fix conversion fuction #158

wants to merge 43 commits into from

Conversation

LorenzoMerotto
Copy link
Collaborator

Update of conversion function between mouse and human genes
Addition of custom deconvolution with seqimmucc
Fixing minor bugs (#156 )

Copy link

github-actions bot commented Nov 17, 2023

@github-actions github-actions bot temporarily deployed to pull request November 17, 2023 14:16 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 17, 2023 14:44 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 17, 2023 16:25 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 20, 2023 14:03 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 20, 2023 14:51 Inactive
@LorenzoMerotto
Copy link
Collaborator Author

@grst do you have an idea about what's wrong here? I assumed we needed to update the version of conda but this looks already like the latest one

@github-actions github-actions bot temporarily deployed to pull request November 22, 2023 16:01 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 22, 2023 16:56 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 22, 2023 17:02 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 23, 2023 08:49 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 23, 2023 08:53 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 23, 2023 08:59 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 23, 2023 09:03 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 23, 2023 09:12 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 23, 2023 10:10 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 23, 2023 10:21 Inactive
@LorenzoMerotto
Copy link
Collaborator Author

@grst I'm waiting to merge to see what you think about that

Copy link
Collaborator

@grst grst left a comment

Choose a reason for hiding this comment

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

Sorry, I missed your comment about conda in all the github notifications.

I have a couple of questions, but looks good otherwise

@@ -29,7 +29,7 @@ custom_deconvolution_methods <- c(
"EPIC" = "epic",
"CIBERSORT" = "cibersort",
"ConsensusTME" = "consensus_tme",
"BASE" = "base"
"seqImmuCC" = "seqimmucc"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happened to BASE?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing a method is a breaking change that should only happen with a good reason (+ major version bump + explanation in changelog)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not removing a method here, I'm just removing the possibility to use it with a custom matrix (this is due to an error in my implementation)

Copy link
Collaborator

Choose a reason for hiding this comment

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

all good then

Comment on lines 334 to 335
rownames <- Reduce(intersect, lapply(list.results, rownames))
list.results <- lapply(list.results, function(x) x[rownames, ])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? And wouldn't you rather want an "outer" join with NAs (or 0s) in places where individual samples don't have a cell-type?

Maybe add a comment, why this was necessary to understand this better in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could be another solution too

@@ -124,26 +124,28 @@ In addition, human-based methods can be used to deconvolute mouse data through t
to the corresponding human orthologues

```R
gene_expression_matrix <- immunedeconv::mouse_genes_to_human(gene_expression_matrix)
gene_expression_matrix <- immunedeconv::convert_human_mouse_genes(gene_expression_matrix, convert_to = "human")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did I get this right that the function wasn't renamed, but just the wrong function reference from the tutorial?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The function was renamed because instead of going from mouse genes to human, now it can go both ways

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is also a breaking change then... Usually what is done in such cases is to keep the old function as an alias to the new one (provided that it has a compatible signature) and show a deprecation warning when it is still used.

@github-actions github-actions bot temporarily deployed to pull request November 24, 2023 15:34 Inactive
@github-actions github-actions bot temporarily deployed to pull request December 13, 2023 08:39 Inactive
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