search

microsoft / syntheseus

Package for Retrosynthetic Planning
https://microsoft.github.io/syntheseus/
MIT License
111 stars 14 forks source link

Fix `dictify` to handle torch tensors #99

Closed mrwnmsr closed 1 week ago

mrwnmsr commented 2 weeks ago

PR fixes that the dictify function does not handle tensors, which can be needed in some occasions.

AustinT commented 1 week ago

Good catch Kris, sorry I completely forgot about not supporting torch as a top-level import. Lesson: don't review PRs when I am very tired 🥲

From: Krzysztof Maziarz @.> Sent: September 2, 2024 11:51 PM To: microsoft/syntheseus @.> Cc: Austin Tripp @.>; Review requested @.> Subject: Re: [microsoft/syntheseus] fix dictify function to handle Tensors (PR #99)

@kmaziarz requested changes on this pull request.

I think this will fail the tests, because the core of the library is supposed to run without optional dependencies such as torch. But equally, I don't think we really need to support tensors here. This came up because one of our internal models had a bug and was returning its prediction probability as a tensor. The solution isn't to support tensors in dictify but rather fix the model 🙂 All of the models in syntheseus return their outputs as plain Python datatypes and I think we can keep it that way.

— Reply to this email directly, view it on GitHubhttps://github.com/microsoft/syntheseus/pull/99#pullrequestreview-2276188403, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFNTKE6B4PSGOZ7QLPZQV7LZUTTXPAVCNFSM6AAAAABNN3ZKPWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENZWGE4DQNBQGM. You are receiving this because your review was requested.Message ID: @.***>

mrwnmsr commented 1 week ago

lets close this then