cbroeckl / RAMClustR

Assigning precursor-product ion relationships in indiscriminant MS/MS data
MIT License
12 stars 16 forks source link

Different results with latest InterpretMSSpectrum #48

Open rickhelmus opened 1 year ago

rickhelmus commented 1 year ago

Hi guys,

Recently the patRoon CI tests seems to be reporting different results for RAMClustR. Among those differences are negative values for neutral masses (RC$M)... It seems there was a new version of MSInterpretSpectrum released, and when I test with the previous release the results seem to be back to normal. Perhaps it is a regression or RC needs updating?

Unfortunately, I couldn't find any issue tracker for MSInterpretSpectrum, but maybe @janlisec can help?

hechth commented 1 year ago

@rickhelmus yeah that is related to an update in interpretMSSpectrum, there is already a new RAMClustR package submitted to CRAN which pins the dependency to an older version of this package.

We will work on an update for the new version in the future.

sneumann commented 1 year ago

Would it make sense to suggest a unit test for interpretMSSpectrum that checks expected behaviour ? Yours, Steffen

janlisec commented 1 year ago

I would be more than happy to implement the RAMClustR tests related to InterpretMSSSpectrum already in my package. When I originally developed IMSS I had no idea about the testthat package, but used it in other projects recently. Maybe you send me the testthat (?) file of RAMClustR related to IMSS?

Bests [jan]

Von: Steffen Neumann @.> Gesendet: Donnerstag, 13. Juli 2023 12:57 An: cbroeckl/RAMClustR @.> Cc: Lisec, Jan @.>; Mention @.> Betreff: Re: [cbroeckl/RAMClustR] Different results with latest InterpretMSSpectrum (Issue #48)

SICHERHEITSHINWEIS: Diese E-Mail wurde von außerhalb an die BAM gesendet. Bitte klicken Sie nicht auf Links oder öffnen Anhänge, bevor Sie nicht den Absender verifiziert haben und sicher sind, dass der Inhalt vertrauenswürdig ist. This message was sent from outside of BAM. Please do not click links or open attachments unless you recognize the sender and know the content is trustworthy.

Would it make sense to suggest a unit test for interpretMSSpectrum that checks expected behaviour ? Yours, Steffen

— Reply to this email directly, view it on GitHubhttps://github.com/cbroeckl/RAMClustR/issues/48#issuecomment-1634036465, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AV2NIQUDUJQQBVZCN7SWBQLXP7H6BANCNFSM6AAAAAA2H4LOTQ. You are receiving this because you were mentioned.Message ID: @.**@.>>

rickhelmus commented 1 year ago

@rickhelmus yeah that is related to an update in interpretMSSpectrum, there is already a new RAMClustR package submitted to CRAN which pins the dependency to an older version of this package.

We will work on an update for the new version in the future.

Hi @hechth,

Thanks for the heads-up! Good to know work is already done to fix this.

sneumann commented 1 year ago

Hi @janlisec , any chance to develop InterpretMSSpectrum on some public git(hub/lab) ? Then one could do a PR. Currently we only have the read-only CRAN copy on GitHub, with currently already one testthat in it. https://github.com/cran/InterpretMSSpectrum/tree/master/tests/testthat Yours, Steffen

janlisec commented 1 year ago

Hi Steffen,

I did set up a git repo for InterpretMSSpectrum as suggested (https://github.com/janlisec/InterpretMSSpectrum). I implemented several tests for the findMAIN function used by RAMClustR. I also checked the testthat file of RAMClustR and reproduced the errors. Basically, the only difference in the output is due to rownames of an attribute dataframe to be character now instead of numbers. I did send @Helge @.***> a mail with more details.

Hope that solves the trouble.

Bests [jan]

Von: Steffen Neumann @.> Gesendet: Freitag, 14. Juli 2023 13:39 An: cbroeckl/RAMClustR @.> Cc: Lisec, Jan @.>; Mention @.> Betreff: Re: [cbroeckl/RAMClustR] Different results with latest InterpretMSSpectrum (Issue #48)

SICHERHEITSHINWEIS: Diese E-Mail wurde von außerhalb an die BAM gesendet. Bitte klicken Sie nicht auf Links oder öffnen Anhänge, bevor Sie nicht den Absender verifiziert haben und sicher sind, dass der Inhalt vertrauenswürdig ist. This message was sent from outside of BAM. Please do not click links or open attachments unless you recognize the sender and know the content is trustworthy.

Hi @janlisechttps://github.com/janlisec , any chance to develop InterpretMSSpectrum on some public git(hub/lab) ? Then one could do a PR. Currently we only have the read-only CRAN copy on GitHub, with currently already one testthat in it. https://github.com/cran/InterpretMSSpectrum/tree/master/tests/testthat Yours, Steffen

— Reply to this email directly, view it on GitHubhttps://github.com/cbroeckl/RAMClustR/issues/48#issuecomment-1635737328, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AV2NIQRTXMZTNQFTYSFOHRLXQEVWNANCNFSM6AAAAAA2H4LOTQ. You are receiving this because you were mentioned.Message ID: @.**@.>>

hechth commented 1 year ago

Hi all - yeah I think I submitted RAMClustR version 1.3.1 to CRAN which depends on InterpretMSSpectrum <=1.3.3, I will work on 1.3.2 which will support the new InterpretMSSpectrum very soon.

I'm not sure why my CRAN submission hasn't seen any comments yet or so, I need to check that.