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.
compute mu #278
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
compute mu #278
Changes from 9 commits
5180eeb
5ff2c23
1826221
e54f9bb
be7c275
9e4bc0d
1879f44
a108205
0bdbbb4
00f36ad
255c603
5492ad8
a4a217d
953e187
dd52ad9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not the name, just chemical formula.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
g/cm*3, not gr/cm^3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make it more readable, don't hardcode numbers but rather variables with obvious names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't need the word warning here. Warnings.warn handles that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a warning message for now. I set density to None if user specifies packing fraction. We can probably replace this with
density=get_density_from_cloud(sample_composition)
. There're some instructions here on how to estimate the density from given chemical formula: https://11bm.xray.aps.anl.gov/absorb/absorb.php. I can try to look into this more.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easy if we know the structure, but I am pretty sure MP has material density as an attribute so a simple API call will give it. We will have to add users adding their API key to the global config.....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Density is just number of atoms/unit cell volume which we know from composition and any CIF file we find, then converted to mass density with the formula, then multiplied by packing fraction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code could be pretty reusable so I expect it will end up in diffpy.utils in the end (but in a future release)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To compactify we want to write this as follows....also, put what you expect, e.g.,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"quartz" is not a omposition, so we don't want this in the tests. It doesn't matter for the test, but let's not confuse readers. Please also fix this below. You can use SiO2 instead.
Also, please can you move the # C2 down a line inside the paren as I tried to illustrate before. thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha. Will fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops there're conflicts - willl fix it