feat: config workflow for wavelength / anode type #169

yucongalicechen · 2025-02-24T23:21:12Z

closes #160
@sbillinge ready for review

codecov · 2025-02-24T23:24:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.31%. Comparing base (bd71eda) to head (b19d44a).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
+ Coverage   99.19%   99.31%   +0.11%     
==========================================
  Files           5        5              
  Lines         249      292      +43     
==========================================
+ Hits          247      290      +43     
  Misses          2        2

Files with missing lines	Coverage Δ
tests/conftest.py	`100.00% <ø> (ø)`
tests/test_tools.py	`98.84% <100.00%> (+0.38%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sbillinge

nice work. Please see inline

sbillinge · 2025-03-04T15:43:26Z

src/diffpy/labpdfproc/tools.py

@@ -159,6 +160,33 @@ def set_input_lists(args):
    return args


+def _load_wavelength_from_config_file(args):
+    """Load wavelength and anode type from config files.
+    It takes cli inputs first, and local config, and then global config.


there should be a blank line before this.

Also, I suggest to make this not private.

sbillinge · 2025-03-15T17:34:45Z

tests/test_tools.py

+        # C3: anode type provided, expect to return args unchanged
+        (["--anode-type", "Mo"], {"wavelength": None, "anode_type": "Mo"}),
+        # C4: both wavelength and anode type provided,
+        # expect to return args unchanged


Is this what we want? I think it should be either wavelength or anode_type. If both are provided raise an exception.

sbillinge · 2025-03-15T17:38:57Z

tests/test_tools.py

+        # C3: anode type provided, expect to return args unchanged
+        (["--anode-type", "Mo"], {"wavelength": None, "anode_type": "Mo"}),
+        # C4: both wavelength and anode type provided,
+        # expect to return args unchanged


I agree that the tests should be the same here, as they are, but we then need to modify the last test in both places.

Also maybe mention in the preamble that the home config file has wavelength of 0.3 and the local config has wavelength of 0.6 to make the test easier to read.

A thought occurred to me that maybe having both wavelength and anode_type specified is tested elsewhere and you do want to return both values here but have it fail in a test elsewhere, but i fthat is the case, say it explicitly in the comment about the test.

sbillinge · 2025-03-15T17:40:14Z

tests/test_tools.py

+    "inputs, expected",
+    [
+        # Test when no config files exist,
+        # expect to return args without modification.


here don't we expect it to run the "create config" workflow if no wavelength/anode_type is specified?

Yeah I think the idea is that this function just updates the correct args. The error will be reported when we set the wavelength etc. I will add a comment here.
For the create config workflow, I think we discussed that we might not want to make it too complicated for users?

I am not sure what we discussed. We created to the "create config workflow" to be as intuitive to use as possible. After that it should just be a choice in apps (a) use it (b) don't use it, so I am not sure I understand your statement about making things not complicated for users.

Getting back to this - I just remembered what we discussed. I think it's a bit complicated to create config workflow here because we want user to enter either wavelength or anode type but not both. So it might be better if we just let the user decide whether to put wavelength/anode type in the config file manyally. Maybe we can load it from args to the config file automatically, if that make things easier?

right, I see. Yes, we could just put it in the docs. In general, we want this behavior set up by say a lab manager (llike a beamline scientist) so that every user who runs the machine doesn't have to enter the value manually. So a complicated command line way of capturing it is probably not warranted. I agree. But let's put it in the docs to merge this.

Sounds good, I added a task in issue #158 to add instructions in docs. I updated the codes to address the comments above.

…ments on loading vs validation

src/diffpy/labpdfproc/tools.py

yucongalicechen · 2025-04-20T17:53:31Z

src/diffpy/labpdfproc/tools.py

+        "wavelength" in global_config or "anode_type" in global_config
+    )
+    if not local_has_data and not global_has_data:
+        print(


Print instructions for manually adding wavelength to config file when it's not found (I reused wording from the create_config function in diffpy.utils)

yucongalicechen · 2025-04-20T17:55:17Z

tests/test_tools.py

+        # otherwise update args with values from the home config file
+        # (wavelength=0.3, no anode type).
+        # This test only checks loading behavior,
+        # not value validation (which is handled by `set_wavelength`).


Added home config info about wavelength/anodetype and clarified that this function only checks loading behavior

yucongalicechen · 2025-04-20T18:22:48Z

tests/test_tools.py

@@ -279,13 +406,13 @@ def test_set_wavelength(inputs, expected):
        (  # C3: invalid anode type
            # expect error asking to specify a valid anode type
            ["--anode-type", "invalid"],
-            f"Anode type not recognized. "
+            f"Anode type 'invalid' not recognized. "


Updated error message slightly to be more informative

yucongalicechen · 2025-04-20T18:27:00Z

Okay, I did some cleanup and updates to match the current muD cli behavior - ready to be review now @sbillinge

sbillinge

very nicely done. Let's user test now and make sure users like the workflow.

sbillinge · 2025-04-21T12:03:04Z

src/diffpy/labpdfproc/tools.py

+    config = local_config if local_has_data else global_config
+    if config:
+        args.wavelength = args.wavelength or config.get("wavelength")
+        args.anode_type = args.anode_type or config.get("anode_type")


this is all very nicely done. I like the coding style very much. Very readable.

yucongalicechen added 3 commits February 22, 2025 22:23

initial commit

411ba59

feat: add tests

bc118ea

docs: add news

c6c8b67

sbillinge reviewed Mar 15, 2025

View reviewed changes

feat: print message if no wavelength info in config; clarify test com…

79dae25

…ments on loading vs validation

yucongalicechen mentioned this pull request Apr 20, 2025

docs: tools module #158

Closed

1 task

yucongalicechen commented Apr 20, 2025

View reviewed changes

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

yucongalicechen commented Apr 20, 2025

View reviewed changes

yucongalicechen added 3 commits April 20, 2025 13:58

Merge branch 'main' into wavelength-config

cf3b37e

Merge branch 'main' into wavelength-config

283c2d3

fix: update tests for updated muD cli behavior

d040e81

yucongalicechen commented Apr 20, 2025

View reviewed changes

remove debug print message

b19d44a

sbillinge reviewed Apr 21, 2025

View reviewed changes

sbillinge merged commit 9618ab0 into diffpy:main Apr 21, 2025
5 checks passed

yucongalicechen mentioned this pull request Apr 21, 2025

docs: update instructions for writing wavelength/anodetype to config file #171

Closed

yucongalicechen deleted the wavelength-config branch April 21, 2025 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: config workflow for wavelength / anode type #169

feat: config workflow for wavelength / anode type #169

yucongalicechen commented Feb 24, 2025

codecov bot commented Feb 24, 2025 •

edited

Loading

sbillinge left a comment

sbillinge Mar 4, 2025

sbillinge Mar 15, 2025

sbillinge Mar 15, 2025

sbillinge Mar 15, 2025

yucongalicechen Mar 15, 2025 •

edited

Loading

sbillinge Mar 17, 2025

yucongalicechen Apr 18, 2025

sbillinge Apr 19, 2025

yucongalicechen Apr 20, 2025

yucongalicechen Apr 20, 2025 •

edited

Loading

yucongalicechen Apr 20, 2025

yucongalicechen Apr 20, 2025

yucongalicechen commented Apr 20, 2025

sbillinge left a comment

sbillinge Apr 21, 2025

feat: config workflow for wavelength / anode type #169

feat: config workflow for wavelength / anode type #169

Conversation

yucongalicechen commented Feb 24, 2025

codecov bot commented Feb 24, 2025 • 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

yucongalicechen Mar 15, 2025 • 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 Apr 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucongalicechen commented Apr 20, 2025

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 24, 2025 •

edited

Loading

yucongalicechen Mar 15, 2025 •

edited

Loading

yucongalicechen Apr 20, 2025 •

edited

Loading