Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: new wavelength workflow #162

Merged
merged 4 commits into from
Feb 6, 2025
Merged

feat: new wavelength workflow #162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
95fee4d
docs: add news
yucongalicechen Jan 31, 2025
36f0cf8
feat: raise error when both wavelength and anode type are specified
yucongalicechen Jan 31, 2025
468f436
docs: improves docstring and test comments
yucongalicechen Feb 3, 2025
1dc6972
fix: removed mu*D requirement for one of the error msg
yucongalicechen Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions news/wavelength-workflow.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
**Added:**

* <news item>

**Changed:**

* Workflow for loading wavelength - raise an error when both wavelength and anode type are specified.

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>
21 changes: 9 additions & 12 deletions src/diffpy/labpdfproc/labpdfprocapp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ def define_arguments():
"in a file with name file_list.txt. "
"If one or more directory is provided, all valid "
"data-files in that directory will be processed. "
"Examples of valid "
"inputs are 'file.xy', 'data/file.xy', "
"Examples of valid inputs are 'file.xy', 'data/file.xy', "
"'file.xy, data/file.xy', "
"'.' (load everything in the current directory), "
"'data' (load everything in the folder ./data), "
Expand All @@ -48,18 +47,16 @@ def define_arguments():
"name": ["-a", "--anode-type"],
"help": (
f"The type of the x-ray source. "
f"Allowed values are {*[known_sources], }. "
f"Allowed values are {*known_sources, }. "
f"Either specify a known x-ray source or specify wavelength."
),
"default": "Mo",
"default": None,
},
{
"name": ["-w", "--wavelength"],
"help": (
"X-ray source wavelength in angstroms. "
"Not needed if the anode-type is specified. "
"This wavelength will override the anode wavelength "
"if both are specified."
"Not needed if the anode-type is specified."
),
"type": float,
},
Expand Down Expand Up @@ -104,7 +101,7 @@ def define_arguments():
f"The method for computing absorption correction. "
f"Allowed methods: {*CVE_METHODS, }. "
f"Default method is polynomial interpolation "
f"if not specified. "
f"if not specified."
),
"default": "polynomial_interpolation",
},
Expand All @@ -121,7 +118,7 @@ def define_arguments():
"For example, facility='NSLS II', "
"'facility=NSLS II', beamline=28ID-2, "
"'beamline'='28ID-2', 'favorite color'=blue, "
"are all valid key=value items. "
"are all valid key=value items."
),
"nargs": "+",
"metavar": "KEY=VALUE",
Expand All @@ -131,7 +128,7 @@ def define_arguments():
"help": (
"Username will be loaded from config files. "
"Specify here only if you want to "
"override that behavior at runtime. "
"override that behavior at runtime."
),
"default": None,
},
Expand All @@ -140,7 +137,7 @@ def define_arguments():
"help": (
"Email will be loaded from config files. "
"Specify here only if you want to "
"override that behavior at runtime. "
"override that behavior at runtime."
),
"default": None,
},
Expand All @@ -149,7 +146,7 @@ def define_arguments():
"help": (
"ORCID will be loaded from config files. "
"Specify here only if you want to "
"override that behavior at runtime. "
"override that behavior at runtime."
),
"default": None,
},
Expand Down
48 changes: 32 additions & 16 deletions src/diffpy/labpdfproc/tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,10 @@ def set_input_lists(args):


def set_wavelength(args):
"""Set the wavelength based on the given anode_type.
If a wavelength is provided,
it will be used, and the anode_type argument will be removed.
"""Set the wavelength based on the given anode_type or wavelength.

First checks from args. If neither is provided,
it attempts to load from local and then global config file.

Parameters
----------
Expand All @@ -172,15 +173,32 @@ def set_wavelength(args):
Raises
------
ValueError
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,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

and xtype is not the two-theta grid,
(2) both are provided,
(3) anode_type is not one of the known sources,
(4) wavelength is non-positive.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@yucongalicechen yucongalicechen Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if args.wavelength is None and args.anode_type is None:
if args.xtype not in ANGLEQUANTITIES:
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, }."
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edited error msg

)
elif args.wavelength is not None and args.anode_type is not None:
raise ValueError(
f"Please provide either a wavelength or an anode type, not both. "
f"Allowed anode types are {*known_sources, }."
)
elif args.anode_type is not None:
matched_anode_type = next(
(
key
Expand All @@ -197,15 +215,12 @@ def set_wavelength(args):
)
args.anode_type = matched_anode_type
args.wavelength = WAVELENGTHS[args.anode_type]
else:
if args.wavelength <= 0:
raise ValueError(
"No valid wavelength. "
"Please rerun specifying a known anode_type "
"or a positive wavelength."
)
else:
delattr(args, "anode_type")
elif args.wavelength is not None and args.wavelength <= 0:
raise ValueError(
"No valid wavelength. "
"Please rerun specifying a known anode_type "
"or a positive wavelength."
)
return args


Expand Down Expand Up @@ -362,7 +377,8 @@ def load_package_info(args):
def preprocessing_args(args):
"""Perform preprocessing on the provided args.
The process includes loading package and user information,
setting input, output, wavelength, xtype, mu*D, and loading user metadata.
setting input, output, wavelength, anode type, xtype, mu*D,
and loading user metadata.

Parameters
----------
Expand Down
50 changes: 31 additions & 19 deletions tests/test_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,7 @@ def test_set_output_directory_bad(user_filesystem):
@pytest.mark.parametrize(
"inputs, expected",
[
# C1: nothing passed in, expect default is Mo
([], {"wavelength": 0.71073, "anode_type": "Mo"}),
# C2: only a valid anode type was entered (case independent),
# C1: only a valid anode type was entered (case independent),
# expect to match the corresponding wavelength
# and preserve the correct case anode type
(["--anode-type", "Mo"], {"wavelength": 0.71073, "anode_type": "Mo"}),
Expand Down Expand Up @@ -239,45 +237,52 @@ def test_set_output_directory_bad(user_filesystem):
["--anode-type", "cuka1"],
{"wavelength": 1.54056, "anode_type": "CuKa1"},
),
# C3: only a valid wavelength was entered,
# C2: a valid wavelength was entered,
# expect to include the wavelength only and anode type is None
(["--wavelength", "0.25"], {"wavelength": 0.25, "anode_type": None}),
# C4: both valid anode type and wavelength were entered,
# expect to remove the anode type and preserve wavelength only
(
["--wavelength", "0.25", "--anode-type", "Ag"],
{"wavelength": 0.25, "anode_type": None},
),
# C3: nothing passed in, but mu*D was provided and xtype is on tth
# expect wavelength and anode type to be None
# and program proceeds without error
([], {"wavelength": None, "anode_type": None}),
],
)
def test_set_wavelength(inputs, expected):
cli_inputs = ["2.5", "data.xy"] + inputs
actual_args = get_args(cli_inputs)
actual_args = set_wavelength(actual_args)
assert actual_args.wavelength == expected["wavelength"]
assert getattr(actual_args, "anode_type", None) == expected["anode_type"]
assert actual_args.anode_type == expected["anode_type"]


@pytest.mark.parametrize(
"inputs, expected_error_msg",
[
(
( # C1: nothing passed in, xtype is not on tth
# expect error asking for either wavelength or anode type
["--xtype", "q"],
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, }.",
),
( # C2: both wavelength and anode type were specified
# expect error asking not to specify both
["--wavelength", "0.7", "--anode-type", "Mo"],
f"Please provide either a wavelength or an anode type, not both. "
f"Allowed anode types are {*known_sources, }.",
),
( # C3: invalid anode type
# expect error asking to specify a valid anode type
["--anode-type", "invalid"],
f"Anode type not recognized. "
f"Please rerun specifying an anode_type from {*known_sources, }.",
),
(
( # C4: invalid wavelength
# expect error asking to specify a valid wavelength or anode type
["--wavelength", "0"],
"No valid wavelength. "
"Please rerun specifying a known anode_type "
"or a positive wavelength.",
),
(
["--wavelength", "-1", "--anode-type", "Mo"],
"No valid wavelength. "
"Please rerun specifying a known anode_type "
"or a positive wavelength.",
),
],
)
def test_set_wavelength_bad(inputs, expected_error_msg):
Expand Down Expand Up @@ -502,6 +507,11 @@ def test_load_package_info(mocker):


def test_load_metadata(mocker, user_filesystem):
# Test if the function loads args
# (which will be loaded into the header file).
# Expect to include mu*D, anode type, xtype, cve method,
# user-specified metadata, user info, package info, z-scan file,
# and full paths for current input and output directories.
cwd = Path(user_filesystem)
home_dir = cwd / "home_dir"
mocker.patch("pathlib.Path.home", lambda _: home_dir)
Expand All @@ -515,6 +525,8 @@ def test_load_metadata(mocker, user_filesystem):
cli_inputs = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"2.5",
".",
"--anode-type",
"Mo",
"--user-metadata",
"key=value",
"--username",
Expand Down
Loading