improve comments for test_scale_to
#280
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #280 +/- ##
===========================================
+ Coverage 98.68% 100.00% +1.31%
===========================================
Files 8 8
Lines 379 408 +29
===========================================
+ Hits 374 408 +34
+ Misses 5 0 -5
|
tests/test_diffraction_objects.py
Outdated
| @@ -191,7 +191,7 @@ def test_init_invalid_xtype(): | |||
| "org_do_args, target_do_args, scale_inputs, expected", | |||
| [ | |||
| # Test whether scale_to() scales to the expected values | |||
There was a problem hiding this comment.
scales_to() scales to feels a bit circulating?
There was a problem hiding this comment.
Oops yes forgot to change this - thanks!
tests/test_diffraction_objects.py
Outdated
| @@ -191,7 +191,7 @@ def test_init_invalid_xtype(): | |||
| "org_do_args, target_do_args, scale_inputs, expected", | |||
| [ | |||
| # Test whether scale_to() scales to the expected values | |||
| ( # C1: Same x-array and y-array with 2.1 offset, expect yarray shifted by 2.1 offset | |||
| ( # C1: Same x-array and y-array with 2.1 offset, expect y-array to shift up by 2.1 | |||
tests/test_diffraction_objects.py
Outdated
| @@ -212,7 +212,7 @@ def test_init_invalid_xtype(): | |||
| }, | |||
| {"xtype": "tth", "yarray": np.array([4.1, 5.1, 6.1, 7.1, 8.1, 9.1])}, | |||
| ), | |||
| ( # C2: Same length x-arrays with exact x-value match | |||
| ( # C2: Same x-arrays, x-value matches at tth=60, expect y-array to divide by 10 | |||
There was a problem hiding this comment.
If i understand correctly, we compare the yarray_1 of 60 against yarray_2 of 6 (located at xarray of 60) and we expect the y-array to e divided by 10 based on this ratio?
Just comment - since both xarray and yarray use 60 for the first input, it's a bit hard to follow
There was a problem hiding this comment.
Yes! It finds the closest index in each x-array and scales based on that. In this case the scaling factor is 60/6=10 and the y-array for the original diffraction object is divided by 10
tests/test_diffraction_objects.py
Outdated
| ( # C3: Different x-arrays with same length, | ||
| # x-value has closest match at q=0.12 (y=10) and q=0.14 (y=1) | ||
| # for original and target diffraction objects, | ||
| # expect y-array to divide by 10 |
There was a problem hiding this comment.
ok - yarray of 10 and 1 are compared since they are the nearest values at q=0.1. Since the second is 1/10 of the original, we expect y value to be 1/10
tests/test_diffraction_objects.py
Outdated
| @@ -254,7 +257,10 @@ def test_init_invalid_xtype(): | |||
| }, | |||
| {"xtype": "q", "yarray": np.array([1, 2, 4, 6])}, | |||
| ), | |||
| ( # C4: Different x-array lengths with approximate x-value match | |||
| ( # C4: Different x-array lengths | |||
There was a problem hiding this comment.
okay - clear, I understand the function now. yeah for C2, it was hard for me to understand what the function does by reading the test alone since numbers were similiar but other cases are clear.
There was a problem hiding this comment.
Thanks! I'll edit C2
There was a problem hiding this comment.
@yucongalicechen pls see my comments - thanks a lot Yucong!
|
@sbillinge ready for review |
There was a problem hiding this comment.
This is great, thanks @yucongalicechen
My one request might be to reorder the cases so the offset goes to the end. In general, I think that things would be easiest to understand if we test the main obvious functionality first, then edge cases later. This way, the reader first gets the idea what the function is supposed to be doing when everything is working first, then starts to dig into funky edge-cases and unusual behavior later when they already have a clear understanding of the main purpose of the function.
@bobleesj maybe we can add this to our "best practices"? What do you think. Here the offset is a funky extra functionality that I would rather be at the end.
@sbillinge Yup added to our gitlab page |
closes #274
@bobleesj do you wanna give it a read first to see if the comments make sense to you?