Updated site fingerprints #136
Conversation
nisse3000
commented
Feb 13, 2018
nisse3000
commented
- Incorporated new and optimized OPs.
- Added unit tests for CrystalSiteFingerprint.
- Got rid of long if-elif blocks in some presets that use NearNeighbor classes.
- Added maximum_distance_factor in ChemEnvFingerprint, which is hoped to speed up computations.
- Updated pymatgen requirements.
…ocalStructOrderParas
…upposed to speed things up.
|
@computron, any suggestions/comments/criticism regarding this PR? |
|
Right now it conflicts with the main branch Haven't had time to look at it yet (submitted <1 day ago) - do you need it merged right away? If so I can look at it after the conflicts are resolved, just let me know |
|
@computron, it didn't conflict yesterday... Sorry. Will work on the conflicts. |
|
(let me know when ready for review) |
|
@computron, the tests passed. Have a look if you have time. It's not super urgent. It's only that I did the pull request yesterday before someone made changes to site.py that caused the conflicts. |
matminer/featurizers/site.py
Outdated
| with open(os.path.join(os.path.dirname( | ||
| pymatgen.analysis.__file__), 'cn_opt_paras.yaml'), 'r') as f: | ||
| cn_motif_op_paras = yaml.safe_load(f) | ||
| f.close() |
There was a problem hiding this comment.
if you use with open(), you don't need to do f.close() (that's the primary point of using with open() instead f.open())
matminer/featurizers/site.py
Outdated
| @@ -33,6 +36,18 @@ | |||
|
|
|||
| from matminer.featurizers.stats import PropertyStats | |||
|
|
|||
| cn_motif_op_paras = {} | |||
There was a problem hiding this comment.
note that we usually abbreviate as params rather than paras. Params is used >1200 times in the various repos, paras is used ~125 times, mostly by the local env code
matminer/featurizers/site.py
Outdated
| @@ -172,38 +186,28 @@ class OPSiteFingerprint(BaseFeaturizer): | |||
| default: True). | |||
| """ | |||
|
|
|||
| def __init__(self, optypes=None, dr=0.1, ddr=0.01, ndr=1, dop=0.001, | |||
| def __init__(self, targets=None, dr=0.1, ddr=0.01, ndr=1, dop=0.001, | |||
There was a problem hiding this comment.
We often use targets to mean target elements to bond to, e.g. the some of the NN codes do this. Maybe target_motifs?
|
I put some minor comments, mainly related to naming |
|
The only thing I didn't change (yet) is the file name from cn_opt_paras.yaml to cn_opt_params.yaml because that needs to be done in a pymatgen clean up. |
|
Thanks! |
