search

materialsproject / crystaltoolkit

Crystal Toolkit is a framework for building web apps for materials science and is currently powering the new Materials Project website.
https://docs.crystaltoolkit.org
Other
150 stars 59 forks source link

`Renderable` class as alternative to monkey patching #76

Open shyamd opened 5 years ago

shyamd commented 5 years ago

It appears the conversion of objects to scenes is monkey patched into place. There should be a better way to do this.

mkhorton commented 5 years ago

Thinking of moving it to pymatgen. Monkey patched right now to facilitate an easy move

On Wed, Jul 17, 2019 at 08:12, Shyam Dwaraknath notifications@github.com wrote:

It appears the conversion of objects to scenes is monkey patched into place. There should be a better way to do this.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/materialsproject/crystaltoolkit/issues/76?email_source=notifications&email_token=AAWWWREO2CRUFDMUDLDXHGLP74ZG3A5CNFSM4IERPYIKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4G7YUILQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWWWRFIV3OLHQR3ESVHQ3TP74ZG3ANCNFSM4IERPYIA .

mkhorton commented 5 years ago

Specifically the get_scene() method for anything that can be displayed in 3D...

On Wed, Jul 17, 2019 at 08:28, Matthew Horton mkhorton@lbl.gov wrote:

Thinking of moving it to pymatgen. Monkey patched right now to facilitate an easy move

On Wed, Jul 17, 2019 at 08:12, Shyam Dwaraknath notifications@github.com wrote:

It appears the conversion of objects to scenes is monkey patched into place. There should be a better way to do this.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/materialsproject/crystaltoolkit/issues/76?email_source=notifications&email_token=AAWWWREO2CRUFDMUDLDXHGLP74ZG3A5CNFSM4IERPYIKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4G7YUILQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWWWRFIV3OLHQR3ESVHQ3TP74ZG3ANCNFSM4IERPYIA .

mkhorton commented 5 years ago

Any thoughts on this btw?

On Wed, Jul 17, 2019 at 08:29, Matthew Horton mkhorton@lbl.gov wrote:

Specifically the get_scene() method for anything that can be displayed in 3D...

On Wed, Jul 17, 2019 at 08:28, Matthew Horton mkhorton@lbl.gov wrote:

Thinking of moving it to pymatgen. Monkey patched right now to facilitate an easy move

On Wed, Jul 17, 2019 at 08:12, Shyam Dwaraknath notifications@github.com wrote:

It appears the conversion of objects to scenes is monkey patched into place. There should be a better way to do this.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/materialsproject/crystaltoolkit/issues/76?email_source=notifications&email_token=AAWWWREO2CRUFDMUDLDXHGLP74ZG3A5CNFSM4IERPYIKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4G7YUILQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWWWRFIV3OLHQR3ESVHQ3TP74ZG3ANCNFSM4IERPYIA .

shyamd commented 5 years ago

Yeah, i don't like the idea of putting the get_scene into pymatgen since that makes a bi-directional dependency.

My first thoughts are to make an Abstract Renderable class that also implements a registry pattern. Then we can make adaptors that implement Renderable and register themselves so that we can just something like Renderable.render(object) without having to know what it is.

mkhorton commented 5 years ago

Yeah, I wasn’t intending to make crystal toolkit the dependency for that reason. It’d require moving the scene classes etc into a small standalone package I think.

The abstract Renderable is interesting

On Wed, Jul 17, 2019 at 08:53, Shyam Dwaraknath notifications@github.com wrote:

Yeah, i don't like the idea of putting the get_scene into pymatgen since that makes a bi-directional dependency.

My first thoughts are to make an Abstract Renderable class that also implements a registry pattern. Then we can make adaptors that implement Renderable and register themselves so that we can just something like Renderable.render(object) without having to know what it is.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/materialsproject/crystaltoolkit/issues/76?email_source=notifications&email_token=AAWWWRDCDALZMJCM2ILT453P7456JA5CNFSM4IERPYIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2E3JVY#issuecomment-512341207, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWWWRC7UPKOSOUWLV23ST3P7456JANCNFSM4IERPYIA .

mkhorton commented 2 years ago

Update on this issue. No action has currently been taken, all get_scene() methods for external objects are still in crystal_toolkit.renderables, and patching happens on import of Crystal Toolkit. However, the idea proposed in this issue is still sound, and monkey-patching not ideal, so leaving the issue open to re-visit in future.