-
Notifications
You must be signed in to change notification settings - Fork 11
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
**Added:** | ||
|
||
* Functionality to allow users to specify wavelength or anode type in a diffpy config file. | ||
|
||
**Changed:** | ||
|
||
* <news item> | ||
|
||
**Deprecated:** | ||
|
||
* <news item> | ||
|
||
**Removed:** | ||
|
||
* <news item> | ||
|
||
**Fixed:** | ||
|
||
* <news item> | ||
|
||
**Security:** | ||
|
||
* <news item> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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": "[email protected]", | ||
"owner_orcid": "home_orcid", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import json | ||
import os | ||
import re | ||
from pathlib import Path | ||
|
@@ -6,6 +7,7 @@ | |
|
||
from diffpy.labpdfproc.labpdfprocapp import get_args | ||
from diffpy.labpdfproc.tools import ( | ||
_load_wavelength_from_config_file, | ||
known_sources, | ||
load_metadata, | ||
load_package_info, | ||
|
@@ -195,6 +197,121 @@ def test_set_output_directory_bad(user_filesystem): | |
assert not Path(actual_args.output_directory).is_dir() | ||
|
||
|
||
@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. | ||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this what we want? I think it should be either |
||
( | ||
["--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 args if wavelength or anode type is specified, | ||
# otherwise update args with values from the home config file. | ||
# Results should be the same whether if the home config exists. | ||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
# 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", | ||
[ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should be a blank line before this.
Also, I suggest to make this not private.