compute mu #278

yucongalicechen · 2024-12-26T16:48:34Z

closes #45
@sbillinge ready for review. I can add docs in the same PR once we're happy with the function

codecov · 2024-12-26T16:51:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b0b6676) to head (a108205).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #278   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          405       409    +4     
=========================================
+ Hits           405       409    +4

Files with missing lines	Coverage Δ
tests/test_tools.py	`100.00% <100.00%> (ø)`

sbillinge

please see inline

sbillinge · 2024-12-26T17:04:48Z

src/diffpy/utils/tools.py

+
+    Parameters
+    ----------
+    sample str


I think we would normally do tihs as:

sample: str The chemical formula or the name of the material

i.e., colon and sentencecse

sbillinge · 2024-12-26T17:05:24Z

src/diffpy/utils/tools.py

+        the chemical formula or the name of the material
+    energy float
+        the energy in eV
+    density float or None


density: float, optional, Default is None

sbillinge · 2024-12-26T17:06:47Z

src/diffpy/utils/tools.py

+    energy float
+        the energy in eV
+    density float or None
+        material density in gr/cm^3


this should be "sample mass density" or even better, "mass density of the packed powder/sample" to be clear. the material desnsity is something quite different (mass of atoms in the unit cell divided by unit cell volume)

sbillinge · 2024-12-26T17:07:50Z

src/diffpy/utils/tools.py

@@ -131,3 +133,25 @@ def get_package_info(package_names, metadata=None):
        pkg_info.update({package: importlib.metadata.version(package)})
    metadata["package_info"] = pkg_info
    return metadata
+
+
+def compute_mu_using_xraydb(sample, energy, density=None):


let's change sample to composition or maybe sample_composition.

I think we probably need packing_fraction as an optional arg.

sbillinge · 2024-12-26T17:30:37Z

tests/test_tools.py

@@ -189,3 +189,9 @@ def test_get_package_info(monkeypatch, inputs, expected):
    )
    actual_metadata = get_package_info(inputs[0], metadata=inputs[1])
    assert actual_metadata == expected
+
+
+def test_compute_mu_using_xraydb():


we need a test for no density passed in. Also for packing fraction passed in and also for both passed in. Think about what behavior you want in each of these cases and write a "case" (look elsewhere in test_tools for how we are handling this now)

yucongalicechen · 2024-12-26T20:01:09Z

@sbillinge ready for another review

yucongalicechen · 2024-12-26T20:02:35Z

src/diffpy/utils/tools.py

@@ -131,3 +133,31 @@ def get_package_info(package_names, metadata=None):
        pkg_info.update({package: importlib.metadata.version(package)})
    metadata["package_info"] = pkg_info
    return metadata
+
+
+def compute_mu_using_xraydb(sample_composition, energy, density=None, packing_fraction=1):


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

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.

Also, didn't we discuss to change the name to sample_mass_density?

yucongalicechen · 2024-12-26T20:03:36Z

tests/test_tools.py

+    ({"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),


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)

bobleesj · 2024-12-26T20:05:46Z

tests/test_tools.py

-    sample, energy, density = "ZrO2", 17445.362740959618, 1.009
-    actual_mu = compute_mu_using_xraydb(sample, energy, density=density)
-    assert actual_mu == pytest.approx(1.252, rel=1e-4, abs=0.1)
+params_mu = [


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 params_mu, no need to add "user specific, etc."

@yucongalicechen

@sbillinge if you have also further suggestions to the gitlab doc, please suggest or modify directly

gitlab doc looks pretty good to me. Lot's of great examples to work from.

… mucalc

yucongalicechen · 2024-12-26T20:29:28Z

tests/test_tools.py

+@pytest.mark.parametrize(
+    "inputs, expected_mu",
+    [
+        # Test whether the function returns the correct mu


reorganized test comments

yucongalicechen · 2024-12-26T20:30:17Z

thanks @bobleesj the link is very helpful and very clear to me. I edited the test comments. let me know if anything is unclear!
@sbillinge now ready for another review

sbillinge

please see inline comments. ATM I am just reviewing tests until we get them sorted. I don't really know what "standard density", and it doesn't make too much sense to me, so could you elaborate on that?

sbillinge · 2024-12-26T21:52:27Z

tests/test_tools.py

+        (  # C1: No density or packing fraction provided, expect to compute mu based on standard density
+            {
+                "sample_composition": "H2O",
+                "energy": 10000,


I think we should input energy in keV (so this would be 10, not 10000)

sbillinge · 2024-12-26T21:54:35Z

tests/test_tools.py

+                "sample_composition": "H2O",
+                "energy": 10000,
+                "density": None,
+                "packing_fraction": 1,


According to your case, density and packing fraction should be missing. I think you are passing them in with the default values to mimick this behavior but this is a different test. Just delete them from this input dict to do the correct test.

sbillinge · 2024-12-26T21:57:12Z

tests/test_tools.py

+    ],
+)
+def test_compute_mu_using_xraydb(inputs, expected_mu):
+    actual_mu = compute_mu_using_xraydb(


here you put

def test_compute_mu_using_xraydb(inputs, expected_mu): actual_mu = compute_mu_using_xraydb(**inputs)

which will unpack the input dict. You may have to handle the required args differently, but you can then have input_args and input_kwargs in your paramatrize and unpack them differently.

sbillinge · 2024-12-26T21:59:49Z

tests/test_tools.py

+    "inputs, expected_mu",
+    [
+        # Test whether the function returns the correct mu
+        (  # C1: No density or packing fraction provided, expect to compute mu based on standard density


do we want to allow this? what is "standard density"?

Just density I think - density under the standard conditions. So user can specify either a measured density or packing fraction.

The reason I'm not putting a strict "and/or" for density and packing fraction is that xraydb does not know the density to some materials like ZrO2. In this case user needs to put a density and a packing fraction, or they can put their measured density.

I see, so density is in the database? that is unexpected.

how does xraydb compute the density?

I see, it has a database of some standard materials. So we will need tests for cases where the material can be found and cases where it can't. How many different materials are in that database? The examples show just a few satandard materials (Kapton, quartz, etc.). Is it an extensive database or just a few x-ray standards?

How will you compute it for the cases where it is not in the db?

They have densities for some materials (for example, this returns sample name and density: https://xraypy.github.io/XrayDB/python.html#xraydb.get_materials).
I just reread their docstring and their density refers to the material density, not mass density: https://xraypy.github.io/XrayDB/python.html#xraydb.material_mu.

sbillinge

Please see inline comments.

Also, this is conflicted

sbillinge · 2024-12-26T23:05:38Z

tests/test_tools.py

+    "inputs, expected_mu",
+    [
+        # Test whether the function returns the correct mu
+        (  # C1: No density or packing fraction provided, expect to compute mu based on standard density


I see, it has a database of some standard materials. So we will need tests for cases where the material can be found and cases where it can't. How many different materials are in that database? The examples show just a few satandard materials (Kapton, quartz, etc.). Is it an extensive database or just a few x-ray standards?

sbillinge · 2024-12-26T23:06:01Z

tests/test_tools.py

+    "inputs, expected_mu",
+    [
+        # Test whether the function returns the correct mu
+        (  # C1: No density or packing fraction provided, expect to compute mu based on standard density


How will you compute it for the cases where it is not in the db?

yucongalicechen · 2024-12-27T01:45:28Z

Please see inline comments.

Also, this is conflicted

So I think it only has density of pure elements and common compounds. I used get_materials to print out the list - it's pretty short.

I read through material_mu here: https://github.com/xraypy/XrayDB/blob/master/python/xraydb/materials.py. It uses a weighted sum approach for unknown materials. Here's the related code:

mass_tot, mu = 0.0, 0.0
    for elem, frac in chemparse(formula).items():
        mass  = frac * atomic_mass(elem)
        mu   += mass * mu_elam(elem, energy, kind=kind) # mu_elam is the mu for the given element
        mass_tot += mass
    return density*mu/mass_tot

It looks like it's using mass density so we're good. I'll solve the conflict and add more tests for unknown materials.

yucongalicechen · 2024-12-27T03:06:26Z

src/diffpy/utils/tools.py

+    sample_composition : str
+        The chemical formula or the name of the material.
+    energy : float
+        The energy in keV.


change unit to keV

"The energy of the incident x-rays in keV"

yucongalicechen · 2024-12-27T03:06:34Z

src/diffpy/utils/tools.py

+
+    Computes mu based on the sample composition and energy.
+    User can provide a measured density or an estimated packing fraction.
+    Specifying the density is recommended, though not required for some pure or standard materials.


edit docstring

I think using xrayDB functionality here is not useful. The number of materials is so small, so there is no point to offer this to users as an option. It will just confuse them.

We could offer other approaches:

given a composition, try and get density from COD or Materials Project. For different polymorphs the densities will be different for the same composition, so we would have to handle that.

as a fallback, use a "best effort" atomic density (most tightly packed materials are around 5x10^22 cm^-3)? This would be "magic" which we don't normally like, but if we use this fallback we could give a warning.

yucongalicechen · 2024-12-27T03:06:52Z

tests/test_tools.py

+            },
+            0.5330,
+        ),
+        (  # 2. Unknown material


add unknown material for testing

yucongalicechen · 2024-12-27T03:07:42Z

@sbillinge ready for another review

bobleesj · 2024-12-27T03:23:02Z

tests/test_tools.py

+    ],
+)
+def test_compute_mu_using_xraydb(inputs, expected_mu):
+    actual_mu = compute_mu_using_xraydb(**inputs)


this is beautiful. thanks! @yucongalicechen

sbillinge

please see inline comments.

sbillinge · 2024-12-27T11:07:13Z

src/diffpy/utils/tools.py

@@ -131,3 +133,31 @@ def get_package_info(package_names, metadata=None):
        pkg_info.update({package: importlib.metadata.version(package)})
    metadata["package_info"] = pkg_info
    return metadata
+
+
+def compute_mu_using_xraydb(sample_composition, energy, density=None, packing_fraction=1):


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.

sbillinge · 2024-12-27T11:08:10Z

src/diffpy/utils/tools.py

@@ -131,3 +133,31 @@ def get_package_info(package_names, metadata=None):
        pkg_info.update({package: importlib.metadata.version(package)})
    metadata["package_info"] = pkg_info
    return metadata
+
+
+def compute_mu_using_xraydb(sample_composition, energy, density=None, packing_fraction=1):


Also, didn't we discuss to change the name to sample_mass_density?

sbillinge · 2024-12-27T11:16:49Z

src/diffpy/utils/tools.py

+
+    Computes mu based on the sample composition and energy.
+    User can provide a measured density or an estimated packing fraction.
+    Specifying the density is recommended, though not required for some pure or standard materials.


I think using xrayDB functionality here is not useful. The number of materials is so small, so there is no point to offer this to users as an option. It will just confuse them.

We could offer other approaches:

given a composition, try and get density from COD or Materials Project. For different polymorphs the densities will be different for the same composition, so we would have to handle that.

as a fallback, use a "best effort" atomic density (most tightly packed materials are around 5x10^22 cm^-3)? This would be "magic" which we don't normally like, but if we use this fallback we could give a warning.

sbillinge · 2024-12-27T11:17:27Z

src/diffpy/utils/tools.py

+    sample_composition : str
+        The chemical formula or the name of the material.
+    energy : float
+        The energy in keV.


"The energy of the incident x-rays in keV"

sbillinge · 2024-12-27T11:18:04Z

src/diffpy/utils/tools.py

+    energy : float
+        The energy in keV.
+    density : float, optional, Default is None
+        The mass density of the packed powder/sample in gr/cm^3.


we need more info here about how these will be used, and what happens if someone specifies both.

sbillinge · 2024-12-27T11:22:11Z

tests/test_tools.py

+            },
+            0.5330,
+        ),
+        (  # C2: Packing fraction (=0.5) provided only (only for known material)


reuse my pattern from C1 here.

sbillinge · 2024-12-27T11:23:09Z

tests/test_tools.py

+            },
+            0.2665,
+        ),
+        (  # C3: Density provided only, expect to compute mu based on it


reuse my pattern above.

sbillinge · 2024-12-27T11:24:30Z

tests/test_tools.py

+            },
+            1.252,
+        ),
+        (  # C4: Both density and packing fraction are provided, expect to compute mu based on both


this doesn't make any sense. It will result in two numbers, not one number. We probably want to raise an exception if both are provided.

sbillinge · 2024-12-27T11:25:11Z

tests/test_tools.py

+)
+def test_compute_mu_using_xraydb(inputs, expected_mu):
+    actual_mu = compute_mu_using_xraydb(**inputs)
+    assert actual_mu == pytest.approx(expected_mu, rel=0.01, abs=0.1)


this seems a bit too loose?

sbillinge · 2024-12-27T11:27:10Z

tests/test_tools.py

+            },
+            0.626,
+        ),
+    ],


we eem to be missing "no density, unknown material". Here expected could either be to raise an exception or to do a "best effort" calculation with a warning. I am not sure which is better. It depends how many unknown materials we have.

sbillinge · 2024-12-27T11:30:15Z

@yucongalicechen further to my review comments, I recommend not using the densities from the xrayDB at all and just computing using given densities. We can add another function that is get_density_from_cloud() or something that tries to get it from materials project or COD. This would require there to be a packing fraction specified, because the density we got from MP would be the fully dense density, so "material-density" not "sample-density"

yucongalicechen · 2024-12-27T15:10:08Z

@yucongalicechen further to my review comments, I recommend not using the densities from the xrayDB at all and just computing using given densities. We can add another function that is get_density_from_cloud() or something that tries to get it from materials project or COD. This would require there to be a packing fraction specified, because the density we got from MP would be the fully dense density, so "material-density" not "sample-density"

Just to clarify - this means user must enter density or packing fraction right? Something like what this is doing https://11bm.xray.aps.anl.gov/absorb/absorb.php? I can make another issue on getting density.

bobleesj · 2024-12-27T15:26:59Z

diffpy/diffpy.labpdfproc#146 (comment) - actually I meant to write this comment here in diffpy.utils @yucongalicechen

sbillinge · 2024-12-27T22:07:14Z

@yucongalicechen further to my review comments, I recommend not using the densities from the xrayDB at all and just computing using given densities. We can add another function that is get_density_from_cloud() or something that tries to get it from materials project or COD. This would require there to be a packing fraction specified, because the density we got from MP would be the fully dense density, so "material-density" not "sample-density"

Just to clarify - this means user must enter density or packing fraction right? Something like what this is doing https://11bm.xray.aps.anl.gov/absorb/absorb.php? I can make another issue on getting density.

Yes, either or and it triggers a different workflow.

yucongalicechen and others added 2 commits December 26, 2024 11:47

initial commit

5180eeb

[pre-commit.ci] auto fixes from pre-commit hooks

5ff2c23

sbillinge reviewed Dec 26, 2024

View reviewed changes

yucongalicechen and others added 3 commits December 26, 2024 14:58

add packing fraction, add more tests

1826221

reformat

e54f9bb

[pre-commit.ci] auto fixes from pre-commit hooks

be7c275

yucongalicechen commented Dec 26, 2024

View reviewed changes

bobleesj reviewed Dec 26, 2024

View reviewed changes

yucongalicechen added 2 commits December 26, 2024 15:27

refine docstring and test comments

9e4bc0d

Merge branch 'mucalc' of github.com:yucongalicechen/diffpy.utils into…

1879f44

… mucalc

yucongalicechen commented Dec 26, 2024

View reviewed changes

yucongalicechen mentioned this pull request Dec 26, 2024

add xraydb function to compute muD diffpy/diffpy.labpdfproc#126

Closed

sbillinge reviewed Dec 26, 2024

View reviewed changes

sbillinge reviewed Dec 27, 2024

View reviewed changes

merge from main and add tests for unknown materials

a108205

yucongalicechen commented Dec 27, 2024

View reviewed changes

tests/test_tools.py

},

0.5330,

),

( # 2. Unknown material

Copy link

Contributor Author

yucongalicechen Dec 27, 2024

Choose a reason for hiding this comment

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

add unknown material for testing

bobleesj reviewed Dec 27, 2024

View reviewed changes

sbillinge reviewed Dec 27, 2024

View reviewed changes

yucongalicechen mentioned this pull request Dec 28, 2024

automatic density computation #286

Open

+                          },
+.626,
+                      ),
+                  ],

compute mu #278

Are you sure you want to change the base?

compute mu #278

Conversation

yucongalicechen commented Dec 26, 2024 • edited Loading

codecov bot commented Dec 26, 2024 • edited Loading

Codecov Report

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucongalicechen commented Dec 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucongalicechen commented Dec 26, 2024

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucongalicechen commented Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucongalicechen commented Dec 27, 2024

Choose a reason for hiding this comment

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbillinge commented Dec 27, 2024

yucongalicechen commented Dec 27, 2024

bobleesj commented Dec 27, 2024

sbillinge commented Dec 27, 2024

yucongalicechen commented Dec 26, 2024 •

edited

Loading

codecov bot commented Dec 26, 2024 •

edited

Loading

bobleesj Dec 26, 2024 •

edited

Loading

yucongalicechen commented Dec 27, 2024 •

edited

Loading