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

positional encodings #23

Open
amikki opened this issue Jan 25, 2025 · 3 comments
Open

positional encodings #23

amikki opened this issue Jan 25, 2025 · 3 comments

Comments

@amikki
Copy link

amikki commented Jan 25, 2025

Hi @wehos,
I really like your work and how you organized the codebase 🙂
I have a question about the positional encodings.
I noticed that in the code, the PE are added to the xy coordinates, but it = 0, so I cannot see how the model learned about the cell-cell interactions.

Thanks,
Makky

@wehos
Copy link
Contributor

wehos commented Jan 27, 2025

Hi Makky,

Thanks for your interest! I think you are referring to the same issue here and this is indeed a good catch. #19 (comment)

@amikki
Copy link
Author

amikki commented Feb 7, 2025

Hi @wehos,

Thank you so much for your reply.
I have two relevant questions:
1- What does the parameter 'order_required' in the fit function refer to?
2- In my understanding transformers learn the spatial information from the positional encodings which is zero as we discussed before, so how did CellPLM learn the spatial information or it actually did not learn?
Thank you so much in advance!
Makky

@wehos
Copy link
Contributor

wehos commented Feb 7, 2025

(1) I did not see "order_required" in fit functions' parameters. If you are referring to the inference function as below,

def inference(model, dataloader, split, device, batch_size, order_required=False):

If the cell order is not asked to be preserved, to save time, the model will only infer on specified splits, e.g., validation or test. The returned prediction will not cover all the cells, and the order of predictions is not guaranteed. In this case, we mostly care about the loss instead of the prediction. If we also need the prediction, the dataset split will be ignored and all the cells will be predicted and returned in the original order.

(2) All the codes in this github repo were uploaded at a certain point after we finished our manuscript. I have checked on my private github repo and noticed that the problematic codes were modified on Sep 15, 2023 (our manuscript was first submitted to NeurIPS in May 2023). I believe this change was made when I fine-tuned the model for scRNA-seq data. For now, feel free to remove the comment on spatial transcriptomics data.

Changelog from my private repo (the link is for reference purpose but is not public available):

- pe = self.pe_enc(pe_input)
+ pe = 0.#self.pe_enc(pe_input)
https://github.com/wehos/CellBert/commit/9fc5b1a7bf7b20aa9eac95beba6de067f2684619#diff-286a4c753e046ce11d844e262d909e0ae39ee1fa09b63e02d3be2371cbe78efbL78

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

No branches or pull requests

2 participants