feat: new wavelength workflow #162

yucongalicechen · 2025-01-31T20:35:56Z

closes #161
I will add functionality to load wavelength/anode type from config file in another PR after this is merged
@sbillinge ready for review

src/diffpy/labpdfproc/labpdfprocapp.py

yucongalicechen · 2025-01-31T20:37:23Z

src/diffpy/labpdfproc/tools.py


    Returns
    -------
    args : argparse.Namespace
        The updated arguments with the wavelength.
    """
-    if args.wavelength is None:
+    # first load values from config file


added a comment here. will make another PR for this functionality

When you do that, if possible, let's make things reusable. Either reuse the user machinery in diffpy.utils, or refactor over there so that we can reuse it here for this more general task, rather than putting some bespoke function here. I think one task is maintaining and interacting with a diffpy config file, and specific functions are interacting it for user, orcid and so on, and another it interact with it for wavelength, etc. let's just come up with some "plan" that makes sense.

I think the idea would be similar to the get_user_info function, but since the parameters we pass in are different we cannot reuse it completely. I'm thinking about a function like this:

def load_config_file(args): load local config file load global config file if wavelength/anodetype is present in args: return elif look for wavelength/anode type in local config file: if present: return elif look for wavelength/anode type in global config file: if present: return return args

I think the difference between this and user info is that we cannot just use what's in the config file if user specified anything in args. For example, if user specified wavelength=0.25 while having config file info with anodetype=Mo we don't want to overload anode type to Mo which will cause an error. So I'm not sure how much can be reused. But maybe we can make this more general here, so that this function can be reused in the future?

I guess what I am saying is that there are general tasks (like load_config_file()) that should be in diffpy-utils and do just one thing (in that case, run the config-file loading logic).

Then different apps like yours would use that file, so in your app you would have logic like

config_args = load_config_file() parser_args = load_parser() args = <logic for merging config and cli args>

kind of thing. Your "load_config_file" is doing two things. It is loading the config files in the right order, but it is also running logic on the contents.

codecov · 2025-01-31T20:39:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.21%. Comparing base (f805fe3) to head (1dc6972).
Report is 10 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #162   +/-   ##
=======================================
  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% <100.00%> (ø)`

tests/test_tools.py

sbillinge

super! Please see comments.

src/diffpy/labpdfproc/tools.py

sbillinge · 2025-02-01T16:15:21Z

src/diffpy/labpdfproc/tools.py

-        Raised when input wavelength is non-positive
-        or if input anode_type is not one of the known sources.
+        Raised if:
+        (1) neither is provided, and either mu*D needs to be looked up or


neither what? It is implied but I would make it explicit here.

sbillinge · 2025-02-01T16:17:51Z

src/diffpy/labpdfproc/tools.py


    Returns
    -------
    args : argparse.Namespace
        The updated arguments with the wavelength.
    """
-    if args.wavelength is None:
+    # first load values from config file


When you do that, if possible, let's make things reusable. Either reuse the user machinery in diffpy.utils, or refactor over there so that we can reuse it here for this more general task, rather than putting some bespoke function here. I think one task is maintaining and interacting with a diffpy config file, and specific functions are interacting it for user, orcid and so on, and another it interact with it for wavelength, etc. let's just come up with some "plan" that makes sense.

sbillinge · 2025-02-01T16:20:21Z

src/diffpy/labpdfproc/tools.py

-    if args.wavelength is None:
+    # first load values from config file
+    if args.wavelength is None and args.anode_type is None:
+        if not (args.mud is not None and args.xtype in ANGLEQUANTITIES):


a comment might help readability here. Whilst the logic can, in principle, be parsed from the code, double-negatives and ANDs make it hard, so a brief comment about the desired outcome could help here. Normally, I like code to "self comment" and not put comments, and to simplify code to accomplish this, but here probably there is no way to simplify the logic correctly so a comment is ok.

tests/test_tools.py

sbillinge · 2025-02-01T16:24:03Z

tests/test_tools.py

@@ -515,6 +519,8 @@ def test_load_metadata(mocker, user_filesystem):
    cli_inputs = [


I think this needs the "such and such, expect so and so" treatment

src/diffpy/labpdfproc/labpdfprocapp.py

src/diffpy/labpdfproc/tools.py

tests/test_tools.py

yucongalicechen · 2025-02-03T19:32:01Z

@sbillinge ready for another review!

sbillinge

good progress. Please see my comment. We can move things to a different PR to make it easier to review if you want to make an issue again.....

sbillinge · 2025-02-04T22:07:00Z

src/diffpy/labpdfproc/tools.py


    Returns
    -------
    args : argparse.Namespace
        The updated arguments with the wavelength.
    """
-    if args.wavelength is None:
+    # first load values from config file


I guess what I am saying is that there are general tasks (like load_config_file()) that should be in diffpy-utils and do just one thing (in that case, run the config-file loading logic).

Then different apps like yours would use that file, so in your app you would have logic like

config_args = load_config_file() parser_args = load_parser() args = <logic for merging config and cli args>

kind of thing. Your "load_config_file" is doing two things. It is loading the config files in the right order, but it is also running logic on the contents.

yucongalicechen · 2025-02-04T22:47:59Z

good progress. Please see my comment. We can move things to a different PR to make it easier to review if you want to make an issue again.....

Yes I can work on the config workflow on a separate PR. I see what you mean. I think there's a load_config function in diffpy.utils that we can reuse.

yucongalicechen · 2025-02-05T19:14:26Z

src/diffpy/labpdfproc/tools.py

-        Raised when input wavelength is non-positive
-        or if input anode_type is not one of the known sources.
+        Raised if:
+        (1) neither wavelength or anode type is provided,


I removed the muD requirement here to simplify the logic. I think we will set wavelength before setting muD, so maybe we can raise the error there when we look up the muD?

yucongalicechen · 2025-02-05T19:15:35Z

src/diffpy/labpdfproc/tools.py

+            raise ValueError(
+                f"Please provide a wavelength or anode type "
+                f"because the independent variable axis is not on two-theta. "
+                f"Allowed anode types are {*known_sources, }."


edited error msg

sbillinge · 2025-02-06T01:04:15Z

ok, thanks @yucongalicechen . I got a bit tired of looking at this PR so I merged it. I hope that we have good issues posted for all the unfinished work...... :)

yucongalicechen added 2 commits January 31, 2025 15:12

docs: add news

95fee4d

feat: raise error when both wavelength and anode type are specified

36f0cf8

yucongalicechen commented Jan 31, 2025

View reviewed changes

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

yucongalicechen commented Jan 31, 2025

View reviewed changes

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

yucongalicechen commented Jan 31, 2025

View reviewed changes

tests/test_tools.py Outdated Show resolved Hide resolved

sbillinge reviewed Feb 1, 2025

View reviewed changes

docs: improves docstring and test comments

468f436

yucongalicechen commented Feb 3, 2025

View reviewed changes

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

yucongalicechen commented Feb 3, 2025

View reviewed changes

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

yucongalicechen commented Feb 3, 2025

View reviewed changes

tests/test_tools.py Show resolved Hide resolved

sbillinge reviewed Feb 4, 2025

View reviewed changes

fix: removed mu*D requirement for one of the error msg

1dc6972

yucongalicechen commented Feb 5, 2025

View reviewed changes

sbillinge merged commit 1a263b4 into diffpy:main Feb 6, 2025
5 checks passed

yucongalicechen deleted the wavelength-workflow branch February 7, 2025 00:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new wavelength workflow #162

feat: new wavelength workflow #162

yucongalicechen commented Jan 31, 2025 •

edited

Loading

yucongalicechen Jan 31, 2025

sbillinge Feb 1, 2025

yucongalicechen Feb 3, 2025 •

edited

Loading

yucongalicechen Feb 3, 2025

sbillinge Feb 4, 2025

codecov bot commented Jan 31, 2025 •

edited

Loading

sbillinge left a comment

sbillinge Feb 1, 2025

sbillinge Feb 1, 2025

sbillinge Feb 1, 2025

sbillinge Feb 1, 2025

yucongalicechen commented Feb 3, 2025

sbillinge left a comment

sbillinge Feb 4, 2025

yucongalicechen commented Feb 4, 2025

yucongalicechen Feb 5, 2025

yucongalicechen Feb 5, 2025

sbillinge commented Feb 6, 2025

		@@ -515,6 +519,8 @@ def test_load_metadata(mocker, user_filesystem):
		cli_inputs = [

feat: new wavelength workflow #162

feat: new wavelength workflow #162

Conversation

yucongalicechen commented Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucongalicechen Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 31, 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 commented Feb 3, 2025

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucongalicechen commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbillinge commented Feb 6, 2025

yucongalicechen commented Jan 31, 2025 •

edited

Loading

yucongalicechen Feb 3, 2025 •

edited

Loading

codecov bot commented Jan 31, 2025 •

edited

Loading