TRI-AMDD / htp_md

Shared repo for trajectory analysis and infrastructure development
MIT License
13 stars 8 forks source link

MAT-2976 (random forest model, fix test) #26

Closed danielschweigert-TRI closed 3 years ago

danielschweigert-TRI commented 3 years ago

The issue was that int64 dtypes were filtered out prior to applying the model. In the Windows implementation of rdkit, more of the descriptors are int64 dtype. In the linux implementation still 2 descriptors are int64 and are also not included in the model.

Fix:

@shenggong1996, Excluding these 2 descriptors from the training may or may not have been intended. In case that not, it might be worthwhile to try re-training the model including the two (see code for details on their names)

shenggong1996 commented 3 years ago

Thanks Daniel! I will check the issue and re-do training if necessary.

Best, Sheng

From: @.> Sent: Thursday, September 30, 2021 4:03 PM To: @.> Cc: Sheng @.>; @.> Subject: [TRI-AMDD/htp_md] MAT-2976 (random forest model, fix test) (#26)

The issue was that int64 dtypes were filtered out prior to applying the model. In the Windows implementation of rdkit, more of the descriptors are int64 dtype. In the linux implementation still 2 descriptors are int64 and are also not included in the model.

Fix:

@shenggong1996https://github.com/shenggong1996, Excluding these 2 descriptors from the training may or may not have been intended. In case that not, it might be worthwhile to try re-training the model including the two (see code for details on their names)


You can view, comment on, or merge this pull request online at:

https://github.com/TRI-AMDD/htp_md/pull/26

Commit Summary

File Changes

Patch Links:

- You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/TRI-AMDD/htp_md/pull/26, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AOXFYQ7IODZBLUNNYPYOV53UES7BVANCNFSM5FDDVWGA. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

danielschweigert-TRI commented 3 years ago

@HakyungKwon-TRI Not necessarily - I temporarily added workflows which would trigger the tests on any push to the branch so that I can better troubleshoot the issue without having to open a PR right away. After I was done with the tests, I removed these workflows.