fix: update wavelengths #155

yucongalicechen · 2025-01-27T02:02:23Z

closes #153
@sbillinge ready for review

src/diffpy/labpdfproc/tools.py

codecov · 2025-01-27T02:05:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.21%. Comparing base (983a7b0) to head (63df4ff).
Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #155   +/-   ##
=======================================
  Coverage   99.21%   99.21%           
=======================================
  Files           5        5           
  Lines         254      254           
=======================================
  Hits          252      252           
  Misses          2        2

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

sbillinge

Looks great, please see one comment

tests/test_tools.py

src/diffpy/labpdfproc/tools.py

sbillinge

pls see inline

src/diffpy/labpdfproc/tools.py

sbillinge · 2025-01-27T18:49:01Z

src/diffpy/labpdfproc/tools.py

        args.wavelength = WAVELENGTHS[args.anode_type]
-    else:
+    elif not args.wavelength:


I wonder if the default should be MoKa1. I am not sure which is the most commonly used. Also, it would be good if it could be loaded from a config file. In this way, if it is lab diffractometer, the correct one could be in the config file and loaded by default, but in a different lab, they could update the config file accordingly.

Yeah that makes sense. Is Ka1 more commonly used in general? If so should we have Mo = MoKa1 instead?

This may be a question for @till-schertenleib or probably Pascal. But we should use the one that is most common.

sbillinge · 2025-01-27T19:46:54Z

src/diffpy/labpdfproc/tools.py

-    if args.wavelength:
-        delattr(args, "anode_type")
-    elif args.anode_type:
+    elif not args.wavelength and args.anode_type:


this code seems somewhat convoluted. Is this really the simplest way to do it?

…ength function

sbillinge

As well as the comments here, I was wondering if we could read the anode-type from a config file as an option. This way a lab could set things up with their diffractometer for all their users.

till-schertenleib · 2025-01-28T14:05:34Z

every company has their own file type. Bruker has .brml files which contain all this meta information. But we (everyone) always exports to .xy or .xye where this information is lost.

…

On Tue, 28 Jan 2025 at 14:30, Simon Billinge ***@***.***> wrote: ***@***.**** commented on this pull request. As well as the comments here, I was wondering if we could read the anode-type from a config file as an option. This way a lab could set things up with their diffractometer for all their users. — Reply to this email directly, view it on GitHub <#155 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A2XYJRT3VNCVTH5ZQFP6FXD2M6A77AVCNFSM6AAAAABV5B5AVGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKNZYGI2DKMZSGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Till Schertenleib EPFL, Switzerland Phone: +41 78 729 21 31

sbillinge · 2025-01-28T14:23:01Z

every company has their own file type. Bruker has .brml files which contain all this meta information. But we (everyone) always exports to .xy or .xye where this information is lost.
…
On Tue, 28 Jan 2025 at 14:30, Simon Billinge @.> wrote: @.* commented on this pull request. As well as the comments here, I was wondering if we could read the anode-type from a config file as an option. This way a lab could set things up with their diffractometer for all their users. — Reply to this email directly, view it on GitHub <#155 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2XYJRT3VNCVTH5ZQFP6FXD2M6A77AVCNFSM6AAAAABV5B5AVGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKNZYGI2DKMZSGY . You are receiving this because you were mentioned.Message ID: @.***>
-- Till Schertenleib EPFL, Switzerland Phone: +41 78 729 21 31

Thanks @till-schertenleib . I meant our own config file (that has username in for example). We just store the anode type, and anyone running the software on that computer will get that anode type by default (but can override it of course). Right now, the default if nothing is given is "Mo" which may not be the greatest idea. We need to discuss the workflow we would like users to have, and then implement it.

yucongalicechen · 2025-01-28T21:26:12Z

I put the wavelength in a config json file. Since we get anode types from wavelength file directly, I think we don't need a separate file for that. I pushed my progress.
@till-schertenleib I think the question here is if we want to put the default value Mo or Mo-Ka1?

src/diffpy/labpdfproc/tools.py

sbillinge

please see the comments. This went is a slightly odd direction. Also, we still don't hve teh config file workflow. If you like we could put that on a separate PR. Just make an issue and we can merge this without that.

sbillinge · 2025-01-29T12:19:05Z

src/diffpy/labpdfproc/data/wavelengths_config.json

@@ -0,0 +1,11 @@
+{


I think this should just be in one of the python files. We don't want to have to maintain a dtabase-like json file separately. Is there a reason you moved this from tools.py? This seems like unnecessary infrastructure for us to maintain.

src/diffpy/labpdfproc/tools.py

sbillinge · 2025-01-29T12:21:59Z

src/diffpy/labpdfproc/tools.py

+                "No valid wavelength. Please rerun specifying a known anode_type or a positive wavelength."
+            )
+        else:
+            delattr(args, "anode_type")


do I understand that if the user enters both a valid wavelength and an anode type we default to the wavelength? Is there a good use-case where this makes sense? Otherwise it looks to me like magic and we should just crash if the user provides both (i.e., I can see that they would do htat by mistake, I can't see that they would do that on purpose, though there may be a UC that you have in mind.

Yes so if the user enters only wavelength=0.25 and leaves the anode type empty, it will be automatically loaded as anode type = Mo, but that's wrong. Maybe we can add a check box somewhere to allow user to remove anode type (or automatically remove it when user enters a wavelength, which is kind of what we're doing now)?

I would throw an error if both are specified. Unless you can think of a case where the user would intentionally do that for some reason.

sbillinge

please see inline comment

yucongalicechen · 2025-01-29T21:05:00Z

@sbillinge this PR is ready to be merged now. I've opened new issues for the config workflow

yucongalicechen added 2 commits January 26, 2025 20:57

docs: add news

c5c2da9

fix: fix wavelength values

703b16a

yucongalicechen commented Jan 27, 2025

View reviewed changes

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

sbillinge reviewed Jan 27, 2025

View reviewed changes

tests/test_tools.py Show resolved Hide resolved

fix: make anode type case insensitive, add related tests

64e7c4d

yucongalicechen commented Jan 27, 2025

View reviewed changes

src/diffpy/labpdfproc/tools.py Outdated Show resolved Hide resolved

sbillinge reviewed Jan 27, 2025

View reviewed changes

fix: put WAVELENGTHS into a json file, simplified logic for set_wavel…

89a907a

…ength function

sbillinge reviewed Jan 28, 2025

View reviewed changes

yucongalicechen commented Jan 28, 2025

View reviewed changes

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

sbillinge reviewed Jan 29, 2025

View reviewed changes

fix: merge recut updates from main, remove wavelength json file

63df4ff

This was referenced Jan 29, 2025

feat: add config workflow for wavelength #160

Open

feat: workflow for getting wavelength/anode type #161

Closed

sbillinge merged commit f805fe3 into diffpy:main Jan 30, 2025
5 checks passed

yucongalicechen deleted the wavelength branch January 30, 2025 00:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update wavelengths #155

fix: update wavelengths #155

yucongalicechen commented Jan 27, 2025

codecov bot commented Jan 27, 2025 •

edited

Loading

sbillinge left a comment

sbillinge left a comment

sbillinge Jan 27, 2025

yucongalicechen Jan 28, 2025

sbillinge Jan 28, 2025

sbillinge Jan 27, 2025

sbillinge left a comment

till-schertenleib commented Jan 28, 2025 via email

sbillinge commented Jan 28, 2025

yucongalicechen commented Jan 28, 2025

sbillinge left a comment

sbillinge Jan 29, 2025

sbillinge Jan 29, 2025

yucongalicechen Jan 29, 2025

sbillinge Jan 29, 2025

sbillinge left a comment

yucongalicechen commented Jan 29, 2025

fix: update wavelengths #155

fix: update wavelengths #155

Conversation

yucongalicechen commented Jan 27, 2025

codecov bot commented Jan 27, 2025 • edited Loading

Codecov Report

sbillinge left a 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

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

till-schertenleib commented Jan 28, 2025 via email

sbillinge commented Jan 28, 2025

yucongalicechen commented Jan 28, 2025

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

sbillinge left a comment

Choose a reason for hiding this comment

yucongalicechen commented Jan 29, 2025

codecov bot commented Jan 27, 2025 •

edited

Loading