fix: update functions related to `diffpy.utils` update by yucongalicechen · Pull Request #151 · diffpy/diffpy.labpdfproc

yucongalicechen · 2025-01-03T00:31:39Z

closes #129

…operly

yucongalicechen · 2025-01-03T00:37:36Z

 RADIUS_MM = 1
 N_POINTS_ON_DIAMETER = 300
-TTH_GRID = np.arange(1, 180.1, 0.1)
+TTH_GRID = np.round(np.arange(1, 180.1, 0.1), decimals=1)


np.arrange causes the value at 180 degree to go above 180 slightly because of floating-point precision errors. I think we can round every value to 1 decimal place here.

I ran the interpolation and coefficient lists using np.arrange though... but after rerunning a few angles I believe this rounding error is so small that it can be ignored

mmm., this seems like a slightly dramatic fix to a small edge-case. Is there not a less intrusive fix for this? Let's think about this a bit more.

Yeah, maybe I can round down 180 only?

that's what I had in mind.

yucongalicechen · 2025-01-03T00:41:52Z

@sbillinge ready for review. This PR includes all updates from diffpy.utils. No significant functionality changes. But it's causing a conflict because I removed the mud_calculator.

sbillinge

exciting! please see inline.

sbillinge · 2025-01-03T14:06:28Z

 RADIUS_MM = 1
 N_POINTS_ON_DIAMETER = 300
-TTH_GRID = np.arange(1, 180.1, 0.1)
+TTH_GRID = np.round(np.arange(1, 180.1, 0.1), decimals=1)


mmm., this seems like a slightly dramatic fix to a small edge-case. Is there not a less intrusive fix for this? Let's think about this a bit more.

sbillinge · 2025-01-03T20:22:24Z

you will have to merge in main to resolve the conflict (or add the mud calculator back....)

yucongalicechen · 2025-01-14T18:33:16Z

 N_POINTS_ON_DIAMETER = 300
 TTH_GRID = np.arange(1, 180.1, 0.1)
+# Round down the last element if it's slightly above 180 due to floating point precision
+TTH_GRID[-1] = 180


round down 180 only

maybe 180.00 so it is clear you want to keep it as a float?

codecov · 2025-01-14T18:35:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.27%. Comparing base (a9fbbe5) to head (402677b).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #151      +/-   ##
==========================================
- Coverage   99.31%   99.27%   -0.05%     
==========================================
  Files           6        5       -1     
  Lines         293      275      -18     
==========================================
- Hits          291      273      -18     
  Misses          2        2

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

yucongalicechen · 2025-01-14T18:37:10Z

@sbillinge ready for another review. I fixed the TTH_GRID and resolved the conflicts. I will open another PR on fixing docstrings and tests for username orcid etc. once this is merged.

sbillinge

I didn't quite finish the review and have to run, but I am posting this now so you can work on it.

sbillinge · 2025-01-14T19:50:15Z

+
+**Changed:**
+
+* Updated functions related to changes in `diffpy.utils` version 3.6.0.


Can you try and think more precisely about how to say this. "Updated" is the same as "changed" so the message contains close to zero information.....

sbillinge · 2025-01-14T19:51:15Z

 N_POINTS_ON_DIAMETER = 300
 TTH_GRID = np.arange(1, 180.1, 0.1)
+# Round down the last element if it's slightly above 180 due to floating point precision
+TTH_GRID[-1] = 180


maybe 180.00 so it is clear you want to keep it as a float?

sbillinge · 2025-01-14T19:52:18Z

+from diffpy.utils.diffraction_objects import ANGLEQUANTITIES, QQUANTITIES, XQUANTITIES
+from diffpy.utils.tools import check_and_build_global_config, compute_mud, get_package_info, get_user_info

 WAVELENGTHS = {"Mo": 0.71, "Ag": 0.59, "Cu": 1.54}


I think we want a few more sig figs on these

I found for Mo and Cu the values should be around 0.7107 and 1.5406. I couldn't find the values for Ag though..

sbillinge · 2025-01-14T19:56:19Z

-    config = get_user_info(config)
-    args.username = config["username"]
-    args.email = config["email"]
+    check_and_build_global_config()


since here we are allowing people to pass in this info as an arg, probably the workflow should be

if the arg is set, use the arg and don't even run the config updater

if the arg is not set, run the config updater

run get_user_info passing in the args if they are not None.

sbillinge · 2025-01-14T19:56:56Z

-    args.email = config["email"]
+    check_and_build_global_config()
+    config = get_user_info(owner_name=args.username, owner_email=args.email)
+    args.username = config["owner_name"]


is there a reason you are not using config.get() here? That is usually better practice.

yucongalicechen · 2025-01-14T21:18:54Z

+
+**Changed:**
+
+* Functions related to diffraction_objects and tools modules in `diffpy.utils` to reflect changes in version 3.6.0.


better wording..?

not really. None of the users reading this will have any idea about diffpy.utils 3.6.0.

Something like

* Functions that use DiffractionObject` in `diffpy.utils` to follow the new API.

yucongalicechen · 2025-01-14T21:21:10Z

+from diffpy.utils.tools import check_and_build_global_config, compute_mud, get_package_info, get_user_info

-WAVELENGTHS = {"Mo": 0.71, "Ag": 0.59, "Cu": 1.54}
+WAVELENGTHS = {"Mo": 0.71073, "Ag": 0.55941, "Cu": 1.5406}


Need a little help on the value for Ag. I found it here https://x-server.gmca.aps.anl.gov/cgi/www_dbli.exe?x0hdb=waves but it's a bit off from our original value..

"Cu", "CuKa1", "CuKa1Ka2"
CuKa1 : 1.54056
CuKa1Ka2 : =(1.54056*2 + 1.544398)/3 = 1.54184
Cu : 1.542

@yucongalicechen will need docs to explain what is going on.

yucongalicechen · 2025-01-14T21:23:44Z

@sbillinge ready for some more feedback. The error in the tests is related to the wavelength for Ag.

sbillinge

please see inline

sbillinge · 2025-01-14T22:28:07Z

+
+**Changed:**
+
+* Functions related to diffraction_objects and tools modules in `diffpy.utils` to reflect changes in version 3.6.0.


not really. None of the users reading this will have any idea about diffpy.utils 3.6.0.

Something like

* Functions that use DiffractionObject` in `diffpy.utils` to follow the new API.

sbillinge · 2025-01-14T22:39:07Z

+from diffpy.utils.tools import check_and_build_global_config, compute_mud, get_package_info, get_user_info

-WAVELENGTHS = {"Mo": 0.71, "Ag": 0.59, "Cu": 1.54}
+WAVELENGTHS = {"Mo": 0.71073, "Ag": 0.55941, "Cu": 1.5406}


"Cu", "CuKa1", "CuKa1Ka2"
CuKa1 : 1.54056
CuKa1Ka2 : =(1.54056*2 + 1.544398)/3 = 1.54184
Cu : 1.542

sbillinge · 2025-01-14T22:39:24Z

+from diffpy.utils.tools import check_and_build_global_config, compute_mud, get_package_info, get_user_info

-WAVELENGTHS = {"Mo": 0.71, "Ag": 0.59, "Cu": 1.54}
+WAVELENGTHS = {"Mo": 0.71073, "Ag": 0.55941, "Cu": 1.5406}


@yucongalicechen will need docs to explain what is going on.

yucongalicechen added 6 commits January 2, 2025 19:05

docs: add news

da3340b

fix: correct imports from utils and initialize diffraction objects pr…

8264f17

…operly

fix: update keys in home config file to match changes in utils

f9a9eb1

fix: update diffraction objects in functions

eb5b771

fix: updates for fixtures and tools

e79a2fd

fix: define TTH_GRID more elegantly

84d0e33

yucongalicechen commented Jan 3, 2025

View reviewed changes

Comment thread src/diffpy/labpdfproc/tools.py

yucongalicechen commented Jan 3, 2025

View reviewed changes

Comment thread src/diffpy/labpdfproc/tools.py Outdated

sbillinge reviewed Jan 3, 2025

View reviewed changes

yucongalicechen added 2 commits January 14, 2025 13:28

fix: tweak TTH_GRID and add back check_and_build_global_config

0cad788

merge from main

0534810

yucongalicechen commented Jan 14, 2025

View reviewed changes

sbillinge reviewed Jan 14, 2025

View reviewed changes

yucongalicechen added 2 commits January 14, 2025 15:47

doc: clarify news, clarify TTH_GRID

b479cf2

fix: updated wavelengths, workflow for loading user info

5fecd14

yucongalicechen commented Jan 14, 2025

View reviewed changes

Comment thread src/diffpy/labpdfproc/functions.py

yucongalicechen commented Jan 14, 2025

View reviewed changes

Comment thread src/diffpy/labpdfproc/tools.py

yucongalicechen closed this Jan 14, 2025

yucongalicechen reopened this Jan 14, 2025

sbillinge reviewed Jan 14, 2025

View reviewed changes

docs: update news and wavelength

402677b

yucongalicechen mentioned this pull request Jan 14, 2025

fix: update wavelengths #153

Closed

sbillinge merged commit 08032ab into diffpy:main Jan 14, 2025

yucongalicechen deleted the utils-update branch January 15, 2025 17:59

yucongalicechen mentioned this pull request Jan 15, 2025

getting started documentation #114

Closed


		Changed:

		* Updated functions related to changes in `diffpy.utils` version 3.6.0.


		Changed:

		* Functions related to diffraction_objects and tools modules in `diffpy.utils` to reflect changes in version 3.6.0.

Conversation

yucongalicechen commented Jan 3, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yucongalicechen commented Jan 3, 2025

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbillinge commented Jan 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yucongalicechen commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

yucongalicechen commented Jan 14, 2025

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!

Reviewers

Assignees

Labels

codecov Bot commented Jan 14, 2025 •

edited

Loading

yucongalicechen commented Jan 14, 2025 •

edited

Loading