Skip to content

feat: implement get_density_from_cloud() #333

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yucongalicechen
Copy link
Contributor

closes #331
@sbillinge ready for review

for r in results:
if r.get("sg", "unknown").replace(" ", "").lower() == phase:
return r["structure"].density
print(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Print out available densities and phases. Since COD() handles invalid samples and phases I didn't write any error message here.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's nicely done but it is failing tests. We may want to mock the API call in the test. We generally don't want the tests to rely on an internet connection.

@yucongalicechen
Copy link
Contributor Author

That's nicely done but it is failing tests. We may want to mock the API call in the test. We generally don't want the tests to rely on an internet connection.

I got a chance to play around with the functions a bit more. I also meant to say "space group" instead of "phase" earlier and in the code. I think the issues are:

  1. pymatgen removed some densities by default according to this: Deprecate CifParser.get_structures() in favor of new parse_structures in which primitive defaults to False materialsproject/pymatgen#3419, so it's missing a few common ones like SiO2. I'm wondering if it is better to call MP API directly, just so we have some fallback/default values?
  2. It looks like there can be multiple densities for the same sample composition and space group. Not sure how to pick a default in that case? I think the order is a bit random and that's why the test failed here. We could also give users the option to specify space group & ID?

@sbillinge
Copy link
Contributor

honestly I am not sure I would rely on pymatgen/MP. Why not just make an API call to COD and get the cif and compute the density?

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.

feat: get_density_from_cloud() COD workflow
2 participants