Skip to content

test function for get_angle_index #191

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

Merged
merged 6 commits into from
Dec 7, 2024
Merged

Conversation

yucongalicechen
Copy link
Contributor

issue #186: test function for get_angle_index

@sbillinge ready for review

@yucongalicechen yucongalicechen changed the title initial commit test function for get_angle_index Dec 3, 2024
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.19%. Comparing base (ed9fb03) to head (3dac06a).
Report is 25 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
- Coverage   96.53%   94.19%   -2.35%     
==========================================
  Files           8        8              
  Lines         289      310      +21     
==========================================
+ Hits          279      292      +13     
- Misses         10       18       +8     
Files with missing lines Coverage Δ
tests/test_diffraction_objects.py 80.68% <100.00%> (-5.89%) ⬇️

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

We may want to generalize this to get the index of any x-array, not just angles? Also, I could think of more edge cases so we probably want to use purest.parametrize

the index of the value in the array
"""
if self.on_xtype(xtype) is None:
raise ValueError(_xtype_wmsg(xtype))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can remove this if I let on_xtype() to raise an error for invalid xtypes. Will do that in the other PR.

@yucongalicechen
Copy link
Contributor Author

@sbillinge ready for review!

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

please see inline

# UC1: empty array
(
[0.71, np.array([]), np.array([]), "tth", "tth", 10],
[IndexError, "WARNING: no matching value 10 found in the tth array."],
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 an error or a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be an error. I've edited the error message.

test = DiffractionObject(
wavelength=0.71, xarray=np.array([30, 60, 90]), yarray=np.array([1, 2, 3]), xtype="tth"
)
actual_index = test.get_array_index(xtype="tth", value=30)
Copy link
Contributor

Choose a reason for hiding this comment

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

we need more cases. Here everything is integers and there is a match. What do we want to happen if the value lies between two other values? Return nearest and a warning? What if it lies outside the range of values? Return nearest and a warning? What if it is really far away? Have a threshold after which we raise and error?

Copy link
Contributor Author

@yucongalicechen yucongalicechen Dec 5, 2024

Choose a reason for hiding this comment

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

@sbillinge I addressed this in the new commit. please review.

xtype = self.input_xtype
if self.on_xtype(xtype) is None or len(self.on_xtype(xtype)[0]) == 0:
raise ValueError(
f"The '{xtype}' array is empty. " "Please ensure it is initialized and the correct xtype is used."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current on_xtype function does not raise an error when the specified xtype is invalid. I was thinking about letting that function raise an error in those cases, but it looks like we can handle invalid xtype together with empty array.

i = (np.abs(array - value)).argmin()
nearest_value = np.abs(array[i] - value)
distance = min(np.abs(value - array.min()), np.abs(value - array.max()))
threshold = 0.5 * (array.max() - array.min())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not super sure about the threshold here. I'm thinking about a dynamic threshold within the reasonable range of each array, but this here can cause a bit of problem if the array is sparse.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

please see inline

distance = min(np.abs(value - array.min()), np.abs(value - array.max()))
threshold = 0.5 * (array.max() - array.min())

if nearest_value != 0 and (array.min() <= value <= array.max() or distance <= threshold):
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 rethinking all this. I wonder if we should just return i and be done with it. Raise an error if the array is empty as you do, but then just return i. what do you think? Then it is up to the user to make sure the result makes sense.

@sbillinge
Copy link
Contributor

this needs a news too.

@sbillinge sbillinge merged commit 3aef608 into diffpy:main Dec 7, 2024
4 of 5 checks passed
@yucongalicechen yucongalicechen deleted the angle-test branch December 7, 2024 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants