rename variables in `mud_calculator.py` and write more informative docstring #146

yucongalicechen · 2024-12-26T20:46:26Z

closes #122
@sbillinge ready for review. This PR is just some small edit in functions and docstring.

yucongalicechen · 2024-12-26T20:46:53Z

src/diffpy/labpdfproc/mud_calculator.py

@@ -5,11 +5,11 @@
 from diffpy.utils.parsers.loaddata import loadData


-def _top_hat(x, slit_width):
+def _top_hat(x, half_slit_width):


change slit_width to half_slit_width to avoid confusion

src/diffpy/labpdfproc/mud_calculator.py

yucongalicechen · 2024-12-26T20:47:54Z

src/diffpy/labpdfproc/mud_calculator.py

+
+    This function loads z-scan data and fits it to a model
+    that convolves a top-hat function with I = I0 * e^{-mu * l}.
+    The fitting procedure is run multiple times, and we return the best-fit parameters based on the lowest rmse.


add description to docstring

add a reference to your paper.

Also, I wonder if we should move this function out to diffpy.utils.tools?

yucongalicechen · 2024-12-26T20:48:49Z

src/diffpy/labpdfproc/mud_calculator.py

    """
    x_data, I_data = loadData(filepath, unpack=True)
-    best_mud, _ = min((_compute_single_mud(x_data, I_data) for _ in range(10)), key=lambda pair: pair[1])
+    best_mud, _ = min((_compute_single_mud(x_data, I_data) for _ in range(20)), key=lambda pair: pair[1])


change number of loops from 10 to 20

codecov · 2024-12-26T20:50:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.31%. Comparing base (7f264d2) to head (bd1dd40).
Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #146   +/-   ##
=======================================
  Coverage   99.31%   99.31%           
=======================================
  Files           6        6           
  Lines         293      293           
=======================================
  Hits          291      291           
  Misses          2        2

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

sbillinge

please see inline comments. Since we have the theoretical mu calculator in diffpy.utils, I wonder whether we should move the z-scan mu calculator there too?

sbillinge · 2024-12-26T22:03:44Z

src/diffpy/labpdfproc/mud_calculator.py

@@ -32,7 +32,7 @@ def _model_function(x, diameter, x0, I0, mud, slope):
    return (I0 - slope * x) * np.exp(-mud / diameter * length)


-def _extend_x_and_convolve(x, diameter, slit_width, x0, I0, mud, slope):
+def _extend_x_and_convolve(x, diameter, half_slit_width, x0, I0, mud, slope):


I think these x's should be z's right?

sbillinge · 2024-12-26T22:05:51Z

src/diffpy/labpdfproc/mud_calculator.py

+
+    This function loads z-scan data and fits it to a model
+    that convolves a top-hat function with I = I0 * e^{-mu * l}.
+    The fitting procedure is run multiple times, and we return the best-fit parameters based on the lowest rmse.


add a reference to your paper.

Also, I wonder if we should move this function out to diffpy.utils.tools?

yucongalicechen · 2024-12-26T22:08:53Z

Yeah we can move it - I think this'll be more convenient. Shall I move it under tools or should we add a new module for these two functions?

sbillinge · 2024-12-26T22:53:53Z

Yeah we can move it - I think this'll be more convenient. Shall I move it under tools or should we add a new module for these two functions?

Let's put them in tools for now. They are "tools". We can break them out later if we want.

sbillinge · 2024-12-26T22:54:49Z

Let's finish this PR and merge it, then make another PR that moves it out, which is just a cut and paste.

yucongalicechen · 2024-12-27T02:13:02Z

@sbillinge ready for review

yucongalicechen · 2024-12-27T02:13:20Z

src/diffpy/labpdfproc/mud_calculator.py

@@ -5,96 +5,105 @@
 from diffpy.utils.parsers.loaddata import loadData


-def _top_hat(x, slit_width):
+def _top_hat(z, half_slit_width):


change all x to z

yucongalicechen · 2024-12-27T02:14:01Z

src/diffpy/labpdfproc/mud_calculator.py

    """
    min_radius = -diameter / 2
    max_radius = diameter / 2
-    h = x - x0
+    x = z - z0


change h to x to avoid confusion, since we used h as full slit width in the paper

please see my comment above. We shouldn't use x here. For ease of reading the code, we could just leave it everywhere as (z-z0). This is how it is in the paper I think. If we want to define a new variable it would be better to call it dz, delta_z or z_s or something like that?

src/diffpy/labpdfproc/mud_calculator.py

yucongalicechen · 2024-12-27T02:15:14Z

src/diffpy/labpdfproc/mud_calculator.py

+    The full mathematical details are described in the paper:
+    An ad hoc Absorption Correction for Reliable Pair-Distribution Functions from Low Energy x-ray Sources,
+    Yucong Chen, Till Schertenleib, Andrew Yang, Pascal Schouwink, Wendy L. Queen and Simon J. L. Billinge,
+    in preparation.


add reference

sbillinge

much better. A couple of more comments.

sbillinge · 2024-12-27T10:36:19Z

src/diffpy/labpdfproc/mud_calculator.py

-    compute the model function with the following steps:
-    1. Recenter x to h by subtracting x0 (so that the circle is centered at 0 and it is easier to compute length l)
+    Compute the model function with the following steps:
+    1. Recenter z to x by subtracting z0 (so that the circle is centered at 0 and it is easier to compute length l)


I don't think we are using x correctly here either, right? In the paper, z is the vertical direction centered on the center of the beam. x is the direction of the x-ray beam. We didn't define an x0 (and don't need to) but it could be something like the center of the sample. So I am not sure that it makes sense to say "recentering z to x..."

Yes I will change x to dz and change this docstring here

sbillinge · 2024-12-27T10:42:45Z

src/diffpy/labpdfproc/mud_calculator.py

    """
    min_radius = -diameter / 2
    max_radius = diameter / 2
-    h = x - x0
+    x = z - z0


please see my comment above. We shouldn't use x here. For ease of reading the code, we could just leave it everywhere as (z-z0). This is how it is in the paper I think. If we want to define a new variable it would be better to call it dz, delta_z or z_s or something like that?

sbillinge · 2024-12-27T10:45:33Z

tests/test_mud_calculator.py

            f.write(f"{x}\t{I}\n")

    expected_mud = 3
    actual_mud = compute_mud(file)
-    assert actual_mud == pytest.approx(expected_mud, rel=1e-4, abs=0.1)
+    assert actual_mud == pytest.approx(expected_mud, rel=0.01, abs=0.1)


is there a reason you are making this so loose? In general, it doesn't give me huge confidence if we need it to be so loose t pass tests....

bobleesj · 2024-12-27T15:25:27Z

closes #122 @sbillinge ready for review. This PR is just some small edit in functions and docstring.

General comments for PRs, while there is an issue connected, could you pls also make titles more descriptive so that I (co-maintainers) can later identify the PR for debugging/understanding by scrolling? I like using gh CLI and great titles are great.

(meant to write this comment in diffpy.utils below)

sbillinge · 2024-12-27T22:01:35Z

I will merge this, but @yucongalicechen maybe we could edit the title of the closed PR. Since we are working on "best practices" let's do that. A good thing would be for you two to figure out what the best title would be for future maintainers... So we can learn best practices for docstring titles...

yucongalicechen added 2 commits December 26, 2024 15:06

refine docstring and variable names

664acc0

refine docstring

5c5e1ab

yucongalicechen commented Dec 26, 2024

View reviewed changes

src/diffpy/labpdfproc/mud_calculator.py Show resolved Hide resolved

yucongalicechen commented Dec 26, 2024

View reviewed changes

add news

0ffa99b

sbillinge reviewed Dec 26, 2024

View reviewed changes

yucongalicechen mentioned this pull request Dec 26, 2024

muD calculator for z-scan files diffpy/diffpy.utils#281

Closed

yucongalicechen added 2 commits December 26, 2024 21:10

change variable names

f422317

add reference to paper

bd1dd40

yucongalicechen commented Dec 27, 2024

View reviewed changes

src/diffpy/labpdfproc/mud_calculator.py Show resolved Hide resolved

yucongalicechen commented Dec 27, 2024

View reviewed changes

sbillinge reviewed Dec 27, 2024

View reviewed changes

bobleesj mentioned this pull request Dec 27, 2024

compute mu diffpy/diffpy.utils#278

Merged

sbillinge merged commit e8572fa into diffpy:main Dec 27, 2024
5 checks passed

yucongalicechen changed the title ~~finalize mud_calculator.py~~ rename variables in mud_calculator.py and write more informative docstring Dec 28, 2024

yucongalicechen mentioned this pull request Dec 28, 2024

rename x to dz in mud_calculator.py and decrease error tolerance for tests #147

Merged

rename variables in mud_calculator.py and write more informative docstring #146

rename variables in mud_calculator.py and write more informative docstring #146

Uh oh!

Conversation

yucongalicechen commented Dec 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yucongalicechen commented Dec 26, 2024

Uh oh!

sbillinge commented Dec 26, 2024

Uh oh!

sbillinge commented Dec 26, 2024

Uh oh!

yucongalicechen commented Dec 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobleesj commented Dec 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbillinge commented Dec 27, 2024

Uh oh!

Uh oh!

Uh oh!

rename variables in `mud_calculator.py` and write more informative docstring #146

rename variables in `mud_calculator.py` and write more informative docstring #146

codecov bot commented Dec 26, 2024 •

edited

Loading

bobleesj commented Dec 27, 2024 •

edited

Loading