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

Update machinelearning-functions-PerTurbo.R #146

Merged
merged 1 commit into from Sep 21, 2022
Merged

Conversation

mgerault
Copy link
Contributor

Hello,

I don't know if it's because of the new R version but call max with i like this only return one value; as a consequence all prediction scores from perTurbo are the same for each protein with unknown location. Calling max with apply prevent from this issue.
I didn't check but it's maybe the case with other machine learning functions from pRoloc.

@lgatto
Copy link
Owner

lgatto commented Jun 13, 2022

Thank you @mgerault - would have have some small test data for us to reproduce this and possibly add a unit test.

@samWieczorek, could you also look at this (as well as #147) , given that, as far as I can remember, you originally contributed that code.

@mgerault
Copy link
Contributor Author

mgerault commented Jun 13, 2022

@lgatto I tested it with tan2009r1 dataset from pRolocdata package and other like dunkley2006 etc.
Also, I compared with knn and svm, the results are totally different from perTurbo algorithm --> checked with plot2D and in fData predictions results.

@lgatto
Copy link
Owner

lgatto commented Jun 14, 2022

Thank you very much - I'll look at it asap.

@lgatto
Copy link
Owner

lgatto commented Jun 27, 2022

@samWieczorek - no comment about this?

@lgatto
Copy link
Owner

lgatto commented Jun 27, 2022

Thank you @mgerault - this PR certainly makes sens. I would probably favour

rowMax(ans[i, ])

for clarity and not dropping the i sub-setting from the original code.

@samWieczorek - I will be waiting for your input in this PR and especially #147 before merging.

@lgatto
Copy link
Owner

lgatto commented Sep 21, 2022

Thank you @mgerault, I'll merge now and push to Bioc. But I haven't hear from @samWieczorek, and might consider deprecating the PerTurbo functions in the future, unless you use them.

@lgatto lgatto merged commit 21c7a44 into lgatto:master Sep 21, 2022
@mgerault
Copy link
Contributor Author

mgerault commented Sep 21, 2022 via email

@lgatto
Copy link
Owner

lgatto commented Sep 21, 2022

OK then, thank you for letting me know. Will keep it. Thanks again for fixing the bug!

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.

None yet

2 participants