updated with max_ent - #81
Henry-Plante wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
This should be a test in test_lib.py, not a whole new file
There was a problem hiding this comment.
Looks mostly good. but it looks like you're using an old version of tce-lib here.
After commit 7fb3ac3, we don't use scipy.spatial.KDTree anymore for the neighbor finding. Also, you re-implemented TCECalculator.get_batched_feature_vectors - this was added in the same commit.
| from opt_einsum import contract | ||
| from multiset import Multiset | ||
|
|
||
| # new import for optimized sorting: |
There was a problem hiding this comment.
The import order should look like:
- built-ins (standard library modules like
typing) - third party modules (like
numpy,sparse) - local modules (like
.training)
| LATIN_ALPHABET = "ijklmnopqrstuvwxyz" | ||
|
|
||
| #max_ent funct for optimized matrix sorting | ||
| def maximum_entropy_subset_up_to_size_k( |
There was a problem hiding this comment.
this should probably be a "private" method
i put private in quotes because python doesn't actually have these, but there's a standardized way we can denote it:
- rename it with an underscore (
foo -> _foo)
we also use pdoc to generate documentation and we shouldn't have private functions in the documentation. you can add @private in the doc-string to make sure pdoc doesn't catch it:
| topological_tensors = self.topological_tensors.get(topology_key) | ||
|
|
||
| if topological_tensors is None: | ||
|
|
There was a problem hiding this comment.
old tce-lib version, see the git diff where this error guard doesn't exist
| return obj | ||
|
|
||
| # err handling and center controlling | ||
| def get_batched_feature_vectors( |
There was a problem hiding this comment.
shouldn't exist, see comment on file
with the current version of tce-lib
touched up some documentation
a private function
There was a problem hiding this comment.
by this, i mean that this file should not be committed at all. you'll need to delete it from the git tree
|
|
||
| from dataclasses import dataclass, field | ||
| from typing import Optional, Union | ||
| from typing import Optional, Union, Generator, Sequence # updated import order |
There was a problem hiding this comment.
should not have comments in the lines. no need for the comment here in the imports
| import numpy as np | ||
| from numpy.typing import NDArray | ||
| import sparse | ||
| from scipy.spatial import KDTree |
There was a problem hiding this comment.
new version shouldn't depend on scipy at all, so get rid of this import
|
|
||
| topological_tensors = self.get_topological_tensors(atoms) | ||
|
|
||
| #symbols = np.array(atoms.get_chemical_symbols()) |
|
|
||
|
|
||
| @cite(paper_link=ORIGINAL_PAPER) | ||
| def get_feature_vector( |
There was a problem hiding this comment.
you're missing the @cite decorator here, see the git diff
| r""" | ||
| Finds and selects subsets that maximize the feature entropy. | ||
|
|
||
| Parameters: |
There was a problem hiding this comment.
docstring here is wrong, your annotation is missing atoms_list parameter, and annotates the self parameter. no need to annotate the self parameter
There was a problem hiding this comment.
docstring is still wrong; the atoms_list parameter is not a "matrix of sequence vectors"
| np.testing.assert_array_equal(a1.numbers, a2.numbers) | ||
| np.testing.assert_allclose(a1.positions, a2.positions) | ||
|
|
||
| print("Test passed: Method 1 and Method 2 yield identical results!") |
There was a problem hiding this comment.
no need for the print here, the norm for pytest is to print out info when tests fail
This branch has not been deployed
2 participants
No description provided.