Fix: let DiffractionObject.get_array_index to use xtype from its inputs.
#356
Conversation
tests/test_diffraction_objects.py
Outdated
| @@ -390,7 +390,7 @@ def test_scale_to_bad(org_do_args, target_do_args, scale_inputs): | |||
| }, | |||
| 0, | |||
| ), | |||
| ( | |||
| ( # 2. different xtype | |||
There was a problem hiding this comment.
There was a test for the function. I modified it so that it can capture the bug now.
There was a problem hiding this comment.
I feel that the unfinished discussion is the behavior that we want and the function signature. The way the docstring is written it should be get_array_index(value, xtype=None), i.e., value is required but xtype not. If xtype is None: xtype=_initial_xtype.
Arguably, we could make xtype required. I think that this may make sense. I am not sure the UC where it is implicit. What do you think?
tests/test_diffraction_objects.py
Outdated
| ( # C2: Target value lies in the array, expect the (first) closest | ||
| # index | ||
| # C2: Target value lies in the array, expect the closest index | ||
| ( # 1. same xtype |
There was a problem hiding this comment.
what does "same xtype" mean? same as what?
There was a problem hiding this comment.
It means that the xtype of the original DiffractionObject and the xtype of the data from which the get_array_index gets the array index are the same.
I will modify the comment.
There was a problem hiding this comment.
This may not need to be changed depending on the outcome of the discussion above. I am thinking that we may want to make both variables required. Do you know of a particular UC that requires the other? Do we now anywhere where this is being used actually?
There was a problem hiding this comment.
Do you know of a particular UC that requires the other?
No. I think what the function does is to find the index. xtype is not actually needed.
And if users want to locate a specific q or tth that is different from the _input_xtype, they can use diff_obj.on_xtype("<xtype>"). Also, this might not be a common need.
Do we now anywhere where this is being used actually?
At least in diffpy.utils, it is not referenced. It is just a helper function we provide to users.
I think |
I guess my question is what is the UC for that? Why not make it explicit? Even if you want the index in the original quantity, you can specify it? I mean I made that up in the first place, but I don't really think I know why. Sometimes simpler is better. |
|
On reflection, let's leave it as it is. I don't think there is a strong reason to change it and I may have had a reason I wanted it that way. |
|
Next, for the tests, can we dynamically load a list of xtypes that will automatically update in the future as we add more? Is there a way to do that? It would be good if that list could also be dynamically loaded in the docstring. |
Yes, we can. The allowed xtypes are stored in a variable, and we can use a format string to write those docstrings. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #356 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 9 9
Lines 516 520 +4
=========================================
+ Hits 516 520 +4
🚀 New features to boost your workflow:
|
|
Thanks for catching that and fixing the tests to catch the bad case |
What problem does this PR address
Closes #355
What should the reviewer(s) do
Please check the tests and implementation