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

Most examples apps broken #361

Closed
janosh opened this issue Aug 1, 2023 · 3 comments · Fixed by #362
Closed

Most examples apps broken #361

janosh opened this issue Aug 1, 2023 · 3 comments · Fixed by #362

Comments

@janosh
Copy link
Member

janosh commented Aug 1, 2023

Total example apps:

ls crystal_toolkit/apps/examples/*.py | wc -l
>>> 23  # -2 for utils and __init__

Found 6 broken out of 8 tested

  1. apps/examples/pourbaix.py
  2. apps/examples/structure.py
  3. 💀 apps/examples/phase_diagram.py
  4. 💀 apps/examples/relaxation_trajectory.py
  5. 💀 apps/examples/structure_magnetic.py
  6. 💀 apps/examples/transformations_minimal.py
  7. 💀 apps/examples/transformations.py
  8. 💀 apps/examples/write_structure_screenshot_to_file.py

Below are app screenshots.

apps/examples/pourbaix.py

pourbaix

apps/examples/structure.py

structure

💀 apps/examples/phase_diagram.py

phase_diagram

💀 apps/examples/relaxation_trajectory.py

relaxation_trajectory

💀 apps/examples/structure_magnetic.py

structure_magnetic

💀 apps/examples/transformations_minimal.py

transformations_minimal

💀 apps/examples/transformations.py

transformations

💀 apps/examples/write_structure_screenshot_to_file.py

write_structure_screenshot_to_file

@mkhorton
Copy link
Member

mkhorton commented Aug 1, 2023

Thanks for this summary. I think next step here is to make sure these are working, and add to automated testing (e.g., making sure they run without console errors).

Quick impressions:

  • apps/examples/phase_diagram.py probably affected by Consolidation with effort in web code #264, because a lot of PD improvements were implemented initially in web and have not been upstreamed. It could benefit a lot from the kind of work that was done for the Pourbaix app, which is a lot cleaner comparatively speaking (which is why I expect the Pourbaix demo still works too)
  • apps/examples/structure_magnetic.py looks like a trivial MPRester / document model change
  • apps/examples/write_structure_screenshot_to_file.py this isn't really a real "app" but is a workaround for not being able to generate crystal images via script, since the rendering is done in-browser and requires a WebGL context, nevertheless not sure what the error is here -- the MP team will eventually have to tackle this issue to generate images as new materials are added to the MP website

@tsmathis
Copy link
Collaborator

tsmathis commented Aug 1, 2023

Re: the screenshot generation script: There previously was an imageRequest property within the CrystalToolkitScene component in mp-react-components. At some point in the last two years that property and the associated hooks were removed from mp-react-components, I haven't found the exact commit this occurred at yet so I'm not sure what the reason was for removing it.

I had to re-introduce the imageRequest prop for a recent ad hoc image generation script for new structures. So it could be worth a deeper look.

@mkhorton
Copy link
Member

mkhorton commented Aug 3, 2023

Sorry @tsmathis, sounds frustrating!

I can offer some background on those props for context. Basically, there are two ways of handling a screenshot or 3D model export. Both of these ways can, in principle, be used together:

  1. Trigger the download completely client-side
    • Pros: fast
    • Cons: server cannot retrieve downloaded data
  2. Encode the image/file as a prop, b64-encoded
    • Pros: server can then modify this image (for example, we might want to add a legend, or generate screenshots automatically and upload these to a database)
    • Cons: I felt uneasy about putting a relatively "large" string into a prop

I don't remember exactly, but I think potentially 1. was removed in favor of 2.? I think the capability was always there somehow, it was just a question of whether it was done client-side or server-side.

Regardless of outcome, the ultimate aim was to add both some legend tiles + a credit string to the image via Pillow, as a polite nudge to people to remember to credit the source of the image when it's re-used. This was on the backlog.

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 a pull request may close this issue.

3 participants