-
Notifications
You must be signed in to change notification settings - Fork 21
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 5 commits
5180eeb
5ff2c23
1826221
e54f9bb
be7c275
9e4bc0d
1879f44
a108205
0bdbbb4
00f36ad
255c603
5492ad8
a4a217d
953e187
dd52ad9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
**Added:** | ||
|
||
* function to compute x-ray attenuation coefficient (mu) using XrayDB | ||
|
||
**Changed:** | ||
|
||
* <news item> | ||
|
||
**Deprecated:** | ||
|
||
* <news item> | ||
|
||
**Removed:** | ||
|
||
* <news item> | ||
|
||
**Fixed:** | ||
|
||
* <news item> | ||
|
||
**Security:** | ||
|
||
* <news item> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
numpy | ||
xraydb |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
numpy | ||
xraydb |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
|
||
import pytest | ||
|
||
from diffpy.utils.tools import get_package_info, get_user_info | ||
from diffpy.utils.tools import compute_mu_using_xraydb, get_package_info, get_user_info | ||
|
||
# def _setup_dirs(monkeypatch, user_filesystem): | ||
# home_dir, cwd_dir = user_filesystem.home_dir, user_filesystem.cwd_dir | ||
|
@@ -189,3 +189,27 @@ def test_get_package_info(monkeypatch, inputs, expected): | |
) | ||
actual_metadata = get_package_info(inputs[0], metadata=inputs[1]) | ||
assert actual_metadata == expected | ||
|
||
|
||
params_mu = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please follow the guides provided here? https://gitlab.thebillingegroup.com/resources/group-wiki/-/wikis/Group-standards-for-pytest-and-docstrings add higher level function purpose, use descriptive yet concise variable names, no need to create extra variable of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sbillinge if you have also further suggestions to the gitlab doc, please suggest or modify directly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gitlab doc looks pretty good to me. Lot's of great examples to work from. |
||
# C1: user didn't specify density or packing fraction | ||
({"sample_composition": "H2O", "energy": 10000, "density": None, "packing_fraction": 1}, 0.5330), | ||
# C2: user specified packing fraction only | ||
({"sample_composition": "H2O", "energy": 10000, "density": None, "packing_fraction": 0.5}, 0.2665), | ||
# C3: user specified density only | ||
({"sample_composition": "H2O", "energy": 10000, "density": 0.997, "packing_fraction": 1}, 0.5330), | ||
({"sample_composition": "H2O", "energy": 10000, "density": 0.4985, "packing_fraction": 1}, 0.2665), | ||
# C4: user specified a standard density and a packing fraction | ||
({"sample_composition": "H2O", "energy": 10000, "density": 0.997, "packing_fraction": 0.5}, 0.2665), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added test cases with description. I didn't include the bad cases where the user does not specify a density and xraydb doesn't know the density (this is taken care by xraydb) |
||
] | ||
|
||
|
||
@pytest.mark.parametrize("inputs, expected", params_mu) | ||
def test_compute_mu_using_xraydb(inputs, expected): | ||
actual_mu = compute_mu_using_xraydb( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here you put
which will unpack the input dict. You may have to handle the required args differently, but you can then have |
||
inputs["sample_composition"], | ||
inputs["energy"], | ||
density=inputs["density"], | ||
packing_fraction=inputs["packing_fraction"], | ||
) | ||
assert actual_mu == pytest.approx(expected, rel=0.01, abs=0.1) |
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.
added packing fraction. does this make sense or should we use None instead? I think if we set the default to 1 the code can be more concise
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.
I think setting to None is clearer here. The logic from the perspective of the user is that we either have a packing fraction or we don't. If it is None, you can set it to 1 in the code.
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.
Also, didn't we discuss to change the name to
sample_mass_density
?