crystaltoolkit icon indicating copy to clipboard operation
crystaltoolkit copied to clipboard

Simpler molecule graph support + performance enhancement to XRD plot

Open orionarcher opened this issue 1 year ago • 2 comments

Features

This PR implements two completely separate features:

  1. It cleans up the support for MoleculeGraphs in structure.py
  2. It vectorizes the XRD implementation to improve performance 30x

For feature 1, I had implemented this locally before MoleculeGraph support was added. I think my implementation was a bit cleaner so I propose replacing the current one.

For feature 2, I had implemented this vectorization in mp-web for the IRSpectra. @mattmcdermott suggested I push upstream. This is not an exact port of my code in the IRSpectra, so it's possible there are bugs. I don't have a good testing env set up, so it should be tested before being merged. I probably won't have time to clean this up further but I definitely think it's worth doing!

Checklist

Work-in-progress pull requests are encouraged but please mark your PR as draft.

Usually, the following items should be checked before merging a PR:

  • [ ] Doc strings have been added in the Google docstring format.
  • [ ] Type annotations are highly encouraged. Run mypy path/to/file.py to type-check your code. Type checks are run in CI.
  • [ ] Tests for any new functionality as well as bug fixes, where appropriate.
  • [ ] Create a new Issue for any TODO items that will result from merging the PR.

orionarcher avatar May 05 '23 18:05 orionarcher

Hey @yang-ruoxi. This is a 30x performance improvement over the current implementation but I won't have any time to test it further. I do think it's worth finalizing the implementation if you have the bandwidth! Happy to chat more.

orionarcher avatar May 05 '23 18:05 orionarcher

thanks @orionarcher ! this is very nice. I will test it soon!

yang-ruoxi avatar May 05 '23 20:05 yang-ruoxi