-
Notifications
You must be signed in to change notification settings - Fork 11
load args into DO metadata #82
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
Conversation
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.
nicely done. Please see inline comments.
src/diffpy/labpdfproc/tools.py
Outdated
@@ -213,3 +213,36 @@ def load_package_info(args): | |||
metadata = get_package_info("diffpy.labpdfproc") | |||
setattr(args, "package_info", metadata["package_info"]) | |||
return args | |||
|
|||
|
|||
def _set_anode_type(args): |
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.
this may be doing too much. i.e., "magic"?
I think the ucs are
uc1
- user specifies a standard anode material
- labpdfproc looks up the wavelength
- lpp stores the wavelength and anode type in metadata
UC2
- user specifies a (numeric) wavelength
- lpp stores the (numeric) wavelength in metadata.
I don't thing there is a reverse UC where a numeric value stored and lpp saves the anode type.
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.
I think we only took care of wavelength in the current code. anode type will be Mo if user doesn't specify it. So an input of wavelength=1.54 will produce anode_type=Mo in the current code. Shall I modify this in the wavelength function, or should I add a new function that take care of anode type?
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.
I am not sure what you are saying here. I want to remove the magic: "input of wavelength=1.54 will produce anode_type=Mo"
We don't want to guess where the user got the 1.54 A data from, maybe from a synchrotron?
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.
I mean we might need a separate function (or I can modify the existing wavelength function?) to remove the magic because in the previous issues we only checked that args has the correct wavelength (i.e. we didn't remove the anode type for UC2, but anode type is loaded automatically into the args because it's one of the arguments).
I got your point on not doing the reverse case.
src/diffpy/labpdfproc/tools.py
Outdated
|
||
args = _set_anode_type(args) | ||
metadata = vars(args) | ||
exclude_keys = ["output_correction", "force_overwrite", "input", "input_paths"] |
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.
save these as a global variabl above.
It shouldn't be loaded if it is not specified. If we want we could keep Mo
as default for anode_type but if wavelength is specified, then get remove
it from the args?
…On Thu, Jun 20, 2024 at 4:11 PM Yucong (Alice) Chen < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/diffpy/labpdfproc/tools.py
<#82 (comment)>
:
> @@ -213,3 +213,36 @@ def load_package_info(args):
metadata = get_package_info("diffpy.labpdfproc")
setattr(args, "package_info", metadata["package_info"])
return args
+
+
+def _set_anode_type(args):
I mean we might need a separate function (or I can modify the existing
wavelength function?) to remove the magic because in the previous issues we
only checked that args has the correct wavelength (i.e. we didn't remove
the anode type for UC2, but anode type is loaded automatically into the
args because it's one of the arguments).
I got your point on not doing the reverse case.
—
Reply to this email directly, view it on GitHub
<#82 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAOWUJXYCVROQDIZ65WOYDZIMZPZAVCNFSM6AAAAABJUQTPNSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMZRGI3DQMJWGE>
.
You are receiving this because you commented.Message ID: <diffpy/diffpy.
***@***.***>
--
Simon Billinge
Professor,
Department of Applied Physics and Applied Mathematics
Columbia University
|
Yes I agree. I think I can modify this in the set_wavelength function and check both wavelength and anodetype are correctly loaded? |
I merged main to update correction on anode type. |
yes, make that public and put it in conftest.py so it is easily available to all the testing modules? |
…c function in tools
I put it tools so that we can also use it in labpdfprocapp.py? |
nicely done @yucongalicechen ! |
_set_anode_type
(in tools.py) and_preprocess_args
(in the test) as public functions?