-
Notifications
You must be signed in to change notification settings - Fork 34
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
The difference between paper accepted by ISBM2022 and code in this project #145
Comments
Hi, Thanks for bringing this to our attention, the formulation in the paper is correct but the current Depthcharge implementation is incorrect. We will work on fixing the implementation and release a new model. |
about that, do I think it is a mistake in the paper, but the original version of depthcharge is correct? Because in my understanding, base=min_wavelength / (2 * np.pi) is used to control the minimum resolvable resolution in mz, and scale=max_wavelength/min_wavelength is similar to the 10000 chosen in the original transformer? I don't know if I understand this correctly? If there is a mistake, please point it out, thank you. |
Hi @qiyueliuhuo23 and @liangzhendong123 - thank you so much for looking at our manuscript and code so closely! I've opened a PR fixing the implementation in depthcharge (see wfondrie/depthcharge#28), although the correct equation is a little different:
|
👀so just for sure, the original depthcharge is right? If we do not consider the little difference in cosine? |
I agree with you, according to what @wfondrie have said |
It's embrassing to say, but I found that mistake first, but you didn't mention that at all. It makes me a little unhappy. But its good for you to fix that bug and reply to the question promptly. Thanks a lot. |
Apologies @qiyueliuhuo23, this was simply confusion because both your (default) avatars look pretty similar. Thank you both for pointing out this issue and the discussion. Please see the proposed fixes in wfondrie/depthcharge#28 on how we'll update both the formulation in the paper and the code in depthcharge. We're working on implementing this correctly in Casanovo. |
Hi @qiyueliuhuo23 - sorry for the confusion. I've gone back and updated the comments and PR. 🙏 |
We have now updated the code to use the correct sinusoidal encoding and will release a new version of Casanovo and the corresponding weights after retraining soon. |
In the paper published in ISBM 2022: De novo mass spectrometry peptide sequencing with a transformer model, the input embedding is
![image](https://user-images.githubusercontent.com/45259966/224264980-93696b23-c2e0-49f7-8a06-bb30414ffc35.png)
But in the code in this project, you use depthcharge package (specifically, class MassEncoder from depthcharge/components/encoder.py/MassEncoder) to finish the input embedding, and the specific complement is shown below:
According to the code, the embedding f can be formulated as below:
![image](https://user-images.githubusercontent.com/45259966/224268581-50703c70-17f4-458e-8955-40348dafd2a6.png)
I am wandering why there is a difference between the paper and the code? Is that because the code way can have a better performance? If you could tell me, I would be very grateful. Thanks!
The text was updated successfully, but these errors were encountered: