Skip to content

feat: config workflow for wavelength / anode type #169

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

Merged
merged 8 commits into from
Apr 21, 2025
Merged

feat: config workflow for wavelength / anode type #169

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
411ba59
initial commit
yucongalicechen Feb 23, 2025
bc118ea
feat: add tests
yucongalicechen Feb 24, 2025
c6c8b67
docs: add news
yucongalicechen Feb 24, 2025
79dae25
feat: print message if no wavelength info in config; clarify test com…
yucongalicechen Apr 20, 2025
cf3b37e
Merge branch 'main' into wavelength-config
yucongalicechen Apr 20, 2025
283c2d3
Merge branch 'main' into wavelength-config
yucongalicechen Apr 20, 2025
d040e81
fix: update tests for updated muD cli behavior
yucongalicechen Apr 20, 2025
b19d44a
remove debug print message
yucongalicechen Apr 20, 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-config.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
**Added:**

* Functionality to read wavelength and anode type directly from a diffpy configuration file.

**Changed:**

* <news item>

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>
51 changes: 49 additions & 2 deletions src/diffpy/labpdfproc/tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
XQUANTITIES,
)
from diffpy.utils.tools import (
_load_config,
check_and_build_global_config,
compute_mud,
get_package_info,
Expand Down Expand Up @@ -159,6 +160,52 @@ def set_input_lists(args):
return args


def load_wavelength_from_config_file(args):
"""Load wavelength and anode type from config files.

It prioritizes values in the following order:
1. cli inputs, 2. local config file, 3. global config file.

Parameters
----------
args : argparse.Namespace
The arguments from the parser.

Returns
-------
args : argparse.Namespace
The updated arguments with the updated wavelength and anode type.
"""
global_config = _load_config(Path().home() / "diffpyconfig.json")
local_config = _load_config(Path().cwd() / "diffpyconfig.json")
local_has_data = local_config and (
"wavelength" in local_config or "anode_type" in local_config
)
global_has_data = global_config and (
"wavelength" in global_config or "anode_type" in global_config
)
if not local_has_data and not global_has_data:
print(
Copy link
Collaborator Author

@yucongalicechen yucongalicechen Apr 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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)

"No configuration file was found containing information "
"about the wavelength or anode type. \n"
"You can add the wavelength or anode type "
"to a configuration file on the current computer "
"and it will be automatically associated with "
"subsequent diffpy data by default. \n"
"You will only have to do that once. \n"
"For more information, please refer to www.diffpy.org/"
"diffpy.labpdfproc/examples/toolsexample.html"
)

if args.wavelength or args.anode_type:
return args
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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return args


def set_wavelength(args):
"""Set the wavelength based on the given anode_type or wavelength.

Expand All @@ -174,7 +221,7 @@ def set_wavelength(args):
------
ValueError
Raised if:
(1) neither wavelength or anode type is provided,
(1) neither wavelength or anode type is provided
and xtype is not the two-theta grid,
(2) both are provided,
(3) anode_type is not one of the known sources,
Expand All @@ -185,7 +232,7 @@ def set_wavelength(args):
args : argparse.Namespace
The updated arguments with the wavelength.
"""
# first load values from config file
args = load_wavelength_from_config_file(args)
if args.wavelength is None and args.anode_type is None:
if args.xtype not in ANGLEQUANTITIES:
raise ValueError(
Expand Down
1 change: 1 addition & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ def user_filesystem(tmp_path):
f.write(f"{str(input_dir.resolve() / 'good_data.txt')}\n")

home_config_data = {
"wavelength": 0.3,
"owner_name": "home_username",
"owner_email": "home@email.com",
"owner_orcid": "home_orcid",
Expand Down
127 changes: 127 additions & 0 deletions tests/test_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import json
import os
import re
from pathlib import Path
Expand All @@ -11,6 +12,7 @@
load_package_info,
load_user_info,
load_user_metadata,
load_wavelength_from_config_file,
preprocessing_args,
set_input_lists,
set_mud,
Expand Down Expand Up @@ -195,6 +197,131 @@ def test_set_output_directory_bad(user_filesystem):
assert not Path(actual_args.output_directory).is_dir()


@pytest.mark.parametrize(
"inputs, expected",
[
# Test with only a home config file (no local config),
# expect to return values directly from args
# if either wavelength or anode type is specified,
# 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`).
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 home config info about wavelength/anodetype and clarified that this function only checks loading behavior

# C1: no args, expect to update arg values from home config
([""], {"wavelength": 0.3, "anode_type": None}),
# C2: wavelength provided, expect to return args unchanged
(["--wavelength", "0.25"], {"wavelength": 0.25, "anode_type": None}),
# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

(
["--wavelength", "0.7", "--anode-type", "Mo"],
{"wavelength": 0.7, "anode_type": "Mo"},
),
],
)
def test_load_wavelength_from_config_file_with_home_conf_file(
mocker, user_filesystem, inputs, expected
):
cwd = Path(user_filesystem)
home_dir = cwd / "home_dir"
mocker.patch("pathlib.Path.home", lambda _: home_dir)
os.chdir(cwd)

cli_inputs = ["2.5", "data.xy"] + inputs
actual_args = get_args(cli_inputs)
actual_args = load_wavelength_from_config_file(actual_args)
assert actual_args.wavelength == expected["wavelength"]
assert actual_args.anode_type == expected["anode_type"]


@pytest.mark.parametrize(
"inputs, expected",
[
# Test when a local config file exists,
# expect to return values directly from args
# if either wavelength or anode type is specified,
# otherwise update args with values from the local config file
# (wavelength=0.6, no anode type).
# Results should be the same whether if the home config exists.
# This test only checks loading behavior,
# not value validation (which is handled by `set_wavelength`).
# C1: no args, expect to update arg values from local config
([""], {"wavelength": 0.6, "anode_type": None}),
# C2: wavelength provided, expect to return args unchanged
(["--wavelength", "0.25"], {"wavelength": 0.25, "anode_type": None}),
# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

(
["--wavelength", "0.7", "--anode-type", "Mo"],
{"wavelength": 0.7, "anode_type": "Mo"},
),
],
)
def test_load_wavelength_from_config_file_with_local_conf_file(
mocker, user_filesystem, inputs, expected
):
cwd = Path(user_filesystem)
home_dir = cwd / "home_dir"
mocker.patch("pathlib.Path.home", lambda _: home_dir)
os.chdir(cwd)
local_config_data = {"wavelength": 0.6}
with open(cwd / "diffpyconfig.json", "w") as f:
json.dump(local_config_data, f)

cli_inputs = ["2.5", "data.xy"] + inputs
actual_args = get_args(cli_inputs)
actual_args = load_wavelength_from_config_file(actual_args)
assert actual_args.wavelength == expected["wavelength"]
assert actual_args.anode_type == expected["anode_type"]

# remove home config file, expect the same results
confile = home_dir / "diffpyconfig.json"
os.remove(confile)
assert actual_args.wavelength == expected["wavelength"]
assert actual_args.anode_type == expected["anode_type"]


@pytest.mark.parametrize(
"inputs, expected",
[
# Test when no config files exist,
# expect to return args without modification.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@yucongalicechen yucongalicechen Mar 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

# This test only checks loading behavior,
# not value validation (which is handled by `set_wavelength`).
# C1: no args
([""], {"wavelength": None, "anode_type": None}),
# C1: wavelength provided
(["--wavelength", "0.25"], {"wavelength": 0.25, "anode_type": None}),
# C2: anode type provided
(["--anode-type", "Mo"], {"wavelength": None, "anode_type": "Mo"}),
# C4: both wavelength and anode type provided
(
["--wavelength", "0.7", "--anode-type", "Mo"],
{"wavelength": 0.7, "anode_type": "Mo"},
),
],
)
def test_load_wavelength_from_config_file_without_conf_files(
mocker, user_filesystem, inputs, expected
):
cwd = Path(user_filesystem)
home_dir = cwd / "home_dir"
mocker.patch("pathlib.Path.home", lambda _: home_dir)
os.chdir(cwd)
confile = home_dir / "diffpyconfig.json"
os.remove(confile)

cli_inputs = ["2.5", "data.xy"] + inputs
actual_args = get_args(cli_inputs)
actual_args = load_wavelength_from_config_file(actual_args)
assert actual_args.wavelength == expected["wavelength"]
assert actual_args.anode_type == expected["anode_type"]


@pytest.mark.parametrize(
"inputs, expected",
[
Expand Down
Loading