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
sbillinge 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 1 commit
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
2 changes: 1 addition & 1 deletion news/wavelength-config.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
**Added:**

* Functionality to allow users to specify wavelength or anode type in a diffpy config file.
* Functionality to read wavelength and anode type directly from a diffpy configuration file.

**Changed:**

Expand Down
43 changes: 31 additions & 12 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,11 @@ def set_input_lists(args):
return args


def _load_wavelength_from_config_file(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.

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

Parameters
----------
Expand All @@ -174,16 +176,33 @@ def _load_wavelength_from_config_file(args):
args : argparse.Namespace
The updated arguments with the updated wavelength and anode type.
"""
if args.wavelength or args.anode_type:
return args
global_config = _load_config(Path().home() / "diffpyconfig.json")
local_config = _load_config(Path().cwd() / "diffpyconfig.json")
if local_config:
args.wavelength = local_config.get("wavelength")
args.anode_type = local_config.get("anode_type")
elif global_config:
args.wavelength = global_config.get("wavelength")
args.anode_type = global_config.get("anode_type")
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

@sbillinge sbillinge Apr 21, 2025

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


Expand All @@ -202,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 @@ -213,7 +232,7 @@ def set_wavelength(args):
args : argparse.Namespace
The updated arguments with the wavelength.
"""
args = _load_wavelength_from_config_file(args)
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
28 changes: 19 additions & 9 deletions tests/test_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@

from diffpy.labpdfproc.labpdfprocapp import get_args
from diffpy.labpdfproc.tools import (
_load_wavelength_from_config_file,
known_sources,
load_metadata,
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 @@ -200,9 +200,13 @@ def test_set_output_directory_bad(user_filesystem):
@pytest.mark.parametrize(
"inputs, expected",
[
# Test when only a home config file exists (no local config file),
# expect to return args if wavelength or anode type is specified,
# otherwise update args with values from the home config file.
# 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

@yucongalicechen yucongalicechen Apr 20, 2025

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
Expand All @@ -227,7 +231,7 @@ def test_load_wavelength_from_config_file_with_home_conf_file(

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

Expand All @@ -236,9 +240,13 @@ def test_load_wavelength_from_config_file_with_home_conf_file(
"inputs, expected",
[
# Test when a local config file exists,
# expect to return args if wavelength or anode type is specified,
# otherwise update args with values from the home config file.
# 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
Expand Down Expand Up @@ -266,7 +274,7 @@ def test_load_wavelength_from_config_file_with_local_conf_file(

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

Expand All @@ -282,6 +290,8 @@ def test_load_wavelength_from_config_file_with_local_conf_file(
[
# Test when no config files exist,
# expect to return args without modification.
Copy link
Contributor

@sbillinge sbillinge Mar 15, 2025

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

@sbillinge sbillinge Mar 17, 2025

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

@yucongalicechen yucongalicechen Apr 18, 2025

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

@sbillinge sbillinge Apr 19, 2025

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

@yucongalicechen yucongalicechen Apr 20, 2025

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
Expand All @@ -307,7 +317,7 @@ def test_load_wavelength_from_config_file_without_conf_files(

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

Expand Down
Loading