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

add partial site stats fingerprint #809

Merged
merged 4 commits into from
Aug 12, 2022
Merged

Conversation

jacksund
Copy link
Contributor

@jacksund jacksund commented Apr 16, 2022

(original discussion for this request is located on the discourse page here)

Summary

This adds a PartialsSiteStatsFingerprint.

Much like there is the RDF and partial-RDF fingerprints (partial = broken down by element pairs), this extends the same idea to SiteStats and partial-SiteStats fingerprints (partial = SiteStats broken down by element).

Example use:

from pymatgen.core import Structure
from matminer.featurizers.structure.sites import PartialsSiteStatsFingerprint

# Using rocksalt NaCl as a test structure
s = Structure(
    lattice=[
        [3.485437, 0.0, 2.012318],
        [1.161812, 3.286101, 2.012318],
        [0.0, 0.0, 4.024635],
    ],
    species=["Na", "Cl"],
    coords=[[0.0, 0.0, 0.0], [0.5, 0.5, 0.5]],
)

# Setup mimics partial-RDF fingerprint
f = PartialsSiteStatsFingerprint.from_preset("CrystalNNFingerprint_cn")
f.fit([s])

# Demo of features
labels = f.feature_labels()
fingerprint = f.featurize(s)

@jacksund jacksund marked this pull request as draft April 16, 2022 23:55
@jacksund
Copy link
Contributor Author

@ardunn would you be able to give some feedback for this?

@ardunn
Copy link
Contributor

ardunn commented Apr 27, 2022

Hi @jacksund thanks for the PR! I am quite busy at the moment but after next week I will be able to do a full review on this and we can start merging it in!

In the meantime, I'll at least authorize the CI to run

@ardunn
Copy link
Contributor

ardunn commented Apr 27, 2022

@jacksund Oh, and if you can add a unittest when you get the chance that would be awesome!

@jacksund
Copy link
Contributor Author

Awesome, thank you! I'll fix up the linting and add some tests in the meantime

@jacksund
Copy link
Contributor Author

it actually looks like my linting check didn't even run. The issue seems to be with the CI workflow -- perhaps a change in black api is causing it to fail.

@jacksund
Copy link
Contributor Author

jacksund commented Apr 28, 2022

Would you be able to update your dev-install and contrib docs at some point too? Just little things to help others:

  • For the dev-install, it requires numpy to be installed, so a totally clean environment throws an error
  • For the contrib docs, it wasn't clear how I should run the test suite -- I keep running into issues with optional dependencies like dscribe.

The test suite keeps giving me problems, so I'm using the CI to run tests for now. Hope that's alright 😅

@jacksund
Copy link
Contributor Author

@ardunn I figured out the dev install and the tests should be working for this now. Can you take a look?

@jacksund jacksund marked this pull request as ready for review June 20, 2022 16:36
@jacksund
Copy link
Contributor Author

jacksund commented Aug 5, 2022

@ardunn hey just wanted to check in. Would this PR still be reviewed/accepted?

@ardunn
Copy link
Contributor

ardunn commented Aug 12, 2022

@jacksund Looks good, thanks for the contribution!

@ardunn ardunn merged commit ead30fe into hackingmaterials:main Aug 12, 2022
@jacksund
Copy link
Contributor Author

Awesome! Thank you so much for reading through it 😄

@jacksund
Copy link
Contributor Author

fyi my tests passed locally, but I don't think the CI was ever approved to run/check the final commit. If it throws issues in the CI, just let me know and I'll open a new PR to fix it

@ardunn
Copy link
Contributor

ardunn commented Aug 12, 2022 via email

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 this pull request may close these issues.

2 participants