Porting Moment Computation to Rust and Caching CGR Matrix Computation. #26
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implemented Rust FFI for moments and caching for CGR. "Committing Rust FFI-related files" Changed name of csv file to something shorter. Changed the file system so that the file can be run from the root with python3 -m tests.ex_input Moved all configs to ex_inputs.py file and ran diff check on results from two languages. Implemented max-16-iter rule check for time mode Fixed a bug that occurred when code_timer in ex_input.py was set to None. Deleted unnecessary folder, equistore. Changed the script to perform comparison between different versions Updating isort on __init__ Update tests.yml to include coverage (#8) Update tests.yml Update tests.yml Update tests.yml Update README.md Update tests.yml (#9) * Update tests.yml * Update tests.yml Update tests.yml (#9) * Update tests.yml * Update tests.yml Adding new tests for EDP (#7) * Adding new tests for EDP * Making requisite changes for equistore compatibility * improved code coverage to 99% by testing for 3 additional cases: show_progress=True, multiple frames, and matrix rotations * pass the linter --------- Co-authored-by: Arthur Lin <[email protected]> 5 incorporate normalization factors (#6) * Added (and made default behavior) the ability to orthonormalize features that use the GTO basis. * This involves normalizing the features properly, creating an overlap matrix with orthogonal GTOs, and orthogonalizing the features. * Added relevant tests to test new orthonormality functionality * Added a jupyter notebook displaying how Lowdin Orthonormalization works (on a small gto basis set). --------- Co-authored-by: Rose K. Cersonsky <[email protected]> Co-authored-by: Arthur Lin <[email protected]> minor changes to accomodate new equistore api Updating to be in line with new equistore formatting (#15) Adding progress bar for sanity sake (#14) added warning for passing in integer, and cast any int arguments to f… (#13) * Raise an error when a float is passed for radial_gaussian_width, and added a unit test to ensure error is raised --------- Co-authored-by: Arthur Added code that allows multiple iterations per parameter set. Now uses tqdm to keep track of progress. Removed all internal timing code. The version selection feature still remains for testing purposes. Implemented v2, in which loop for pairwise_ellip_expansion changed (further testing needed). Also, minor changes to output file. Cross-platform support? Needs further testing Added mac move command Fixed file extension error on Mac Moved caching logic to CGR class itself, instead of passing it as a keyword argument. Removed unnecessary keyword argument from single_pass function minor syntax changes for 3.9 added test ellipsoid trimers for performance testing Added subprocess shell command to automatically run makefile. Running makefile automatically while pip install version 2. Added ell-trimers.xyz file. Cross platform support... hopefully. Version 3. Added MANIFEST.in file Changed l_cut to lcut in cg_combine. Fixed the issue of v0's output differing from the original implementation. Some minor cleanup before some more experimental changes Some code cleanup, mostly on the Rust side. Updated equistore to metatensor. Requires changing _classes.py of metatensor. Updated metatensor and rascaline to latest version. Changed import statements, and no _classes.py modification necessary. Removed unused (for now) Rust dependencies Some basic modification for test. Added code to test running time depending on frame numbers. Added code to test running time depending on frame numbers. Changed implementation to determine repetition number with argv Changed CGRCacheList replacement technique from FIFO to Clock (and added documentation) Fixed a detail in Clock algorithm (initial insertion should have replacement flag = 0) Fixed uninitialized _keys in cyclic_list.py Fixed uninitialized _keys in cyclic_list.py Updated metatensor field names to the most recent changes Removing equistore in favor of metatensor (#20) * Removing equistore in favor of metatensor * fixing metatensor.core Adding subtract self to the neighborlist generator (#19) Switching list comp (#24) Switching the code to do the list comp once instead of within each loop Fixed mismatch between Python and Rust compute_moment Cleaned up unused imports and variables. Fixed minor spelling error Deleted unncessary space in monomial_iterator.py Ran isort to satisfy the linter. Changed Rust implementation such that it takes inverse and determinant from Numpy instead of calculating internally. Deleted version control switches and ran isort to satisfy linter. Ran isort again. Fixed a bug that occurs when both error AND inverse are 0. Copied main's contract_pairwise_feat code. Adding ability to override number of radial bases manual override n and added test cases removed extraneous arg Linter Linter Pointing to cutoff radius properly I messed with the history so now I'm restoring it pass linter removed error check restricting independent specification of max_radial and radial_gaussian_width added tests to ensure constant n functionality is correct linter num_radial -> max_radial in edp.py removed changes pertaining to coupling sigma and n to keep PR clean restored original compute_gaussian_parameters function header incorporated necessary changes to make constant n work for both orthonormalization and moments generation pass linter removed deprecated comment made definition of maxradial consistent with num_n and updated tests Update lint.yml (#25) add diff to linter so we can see how files are failing the linter. Used Black formatter
rosecers
force-pushed
the
ycc/pr/rust_and_cache
branch
from
cc7d30c to
916b2e7
Compare
|
Why are the tests failing exactly? It seems things aren't importing properly? |
rosecers
force-pushed
the
ycc/pr/rust_and_cache
branch
3 times, most recently
from
1041c79 to
50c4da4
Compare
rosecers
force-pushed
the
ycc/pr/rust_and_cache
branch
3 times, most recently
from
691ec50 to
ca316dd
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is THE main PR that I was working on. I deleted the version control switch, so it only uses the most recent version of the code. Although I tried to rebase on the most recent main and my cleanup PR (to delete unused variables), I may have made a mistake. Please let me know if you see anything out of order.
Here are the list of main changes:
compute_momentsis ported to Rust. As discussed with Rosy, the Rust version takes inverse and determinant computed bynumpyfor numerical stability reasons. This increases the runtime significantly compared to pure Rust version (4 ~ 10 times slower), but causes less error compared to the original Python version (maximum observed relative error was ~5e-9). Also, the "slower" version is still faster than the original implementation by 6 ~ 45 times on my local machine. I will look into the determinant computation and try to figure out the source of error. I will also run scripts on CHTC to get more objective runtime comparisons.ClebschGordanRealnow uses newCGRCacheList. It uses CLOCK algorithm (originally used for page replacement) to store CG matrices of 5 "most recently used"l_maxvalues [*]. This has an unfortunate(?) side-effect of limitingl_maxvalue to2^31 - 1, but it should not be a problem as we expectl_maxto be much smaller than the maximum value. On my local machine, it divides the number of calls to the constructor by the number of iterations ran with the samel_max. For example, when I ran tests on local machine that performs 5 iterations withl_maxof 10, the number of calls to the constructor went from 3355 to 671 -- a decrease by factor of 5. As such, runtime also decreases by the factor of number of iterations. Note, the cache currently has a capacity of 5. This is an arbitrary decision, and you can increase it later. However, do note that doing so may increase memory usage, especially when it caches matrices for multiple largel_max. In the future, I will try to run some tests on CHTC to determine average runtime improvement caused by caching.pairwise_ellip_expansionfunction has changed. Before, it was looping through every possible pair of species and checking if it is in theneighbor_list.keys. However, I changed so that I directly loop through the elements inneighbor_list.keys. As such,pairwise_ellip_expansionno longer requiresspeciesas one of its arguments.test_compute_moments.pyto compare the moment generation between Rust version and Python version. There are two tests: tests with set parameters and a randomized test. As mentioned in (1), the maximum error observed so far was ~5e-9, so both of my tests usesassert numpy.allclose(x, y, 1e-8)to test for closeness. I have ran several iterations and it has not failed so far.I know it is a big PR, so let me know if you have any questions and/or concerns.
[*]: If you search the CLOCK algorithm, it mimics the "least recently used" replacement policy. However, the algorithm is not exact and it has a chance to replace one of the more recently used entries.