Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add unit tests for peak fitting with real data #310

Merged
merged 17 commits into from
Feb 21, 2020
Merged

Add unit tests for peak fitting with real data #310

merged 17 commits into from
Feb 21, 2020

Conversation

Fahima-Islam
Copy link
Contributor

@Fahima-Islam Fahima-Islam commented Jan 24, 2020 •
edited by peterfpeterson
Loading

Peak fitting unit test hb2b 1060

Fixes #266

peterfpeterson
peterfpeterson requested changes Jan 24, 2020
View reviewed changes
Copy link
Member

@peterfpeterson peterfpeterson left a comment

Choose a reason for hiding this comment

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

Well organized code. Please explain the choice of tolerances. The test would be more useful if they were smaller.

tests/unit/test_peak_fit_HB2B_1060.py Outdated

Data are from the real HB2B data previously reported problematic

Returns
Copy link
Member

Choose a reason for hiding this comment

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

Delete the empty "returns" doc

tests/unit/test_peak_fit_HB2B_1060.py Outdated Show resolved Hide resolved
tests/unit/test_peak_fit_HB2B_1060.py Outdated
param_values_rp, param_errors_rp = fit_result.peakcollections[1].get_native_params()

assert param_values_lp.size == 117, '117 subruns'
assert len(param_values_lp.dtype.names) == 6, '6 effective parameters'
Copy link
Member

Choose a reason for hiding this comment

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

The message should be "6 native parameters"

tests/unit/test_peak_fit_HB2B_1060.py Outdated
assert len(param_values_lp.dtype.names) == 6, '6 effective parameters'

assert param_values_rp.size == 117, '117 subruns'
assert len(param_values_rp.dtype.names) == 6, '6 effective parameters'
Copy link
Member

Choose a reason for hiding this comment

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

same here

tests/unit/test_peak_fit_HB2B_1060.py Outdated
assert fit_result.difference

# peak 'Left'
param_values_lp, param_errors_lp = fit_result.peakcollections[0].get_native_params()
Copy link
Member

Choose a reason for hiding this comment

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

Since you aren't checking the uncertainties

Suggested change
param_values_lp, param_errors_lp = fit_result.peakcollections[0].get_native_params()
param_values_lp, _ = fit_result.peakcollections[0].get_native_params()

tests/unit/test_peak_fit_HB2B_1060.py Outdated
param_values_lp, param_errors_lp = fit_result.peakcollections[0].get_native_params()

# peak 'Right'
param_values_rp, param_errors_rp = fit_result.peakcollections[1].get_native_params()
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Suggested change
param_values_rp, param_errors_rp = fit_result.peakcollections[1].get_native_params()
param_values_rp, _ = fit_result.peakcollections[1].get_native_params()

@peterfpeterson peterfpeterson changed the title #266 Add unit tests for peak fitting with real data Jan 24, 2020
@peterfpeterson
Copy link
Member

The original issue asked for fitting the first three subruns only. You don't need a whole project file for that. Please create a file with x,y,e for these three spectra (ascii or hdf5) to include in the repository and load that instead.

This would be better located in tests/unit/test_peak_fit_engine.py and re-use functionality that exists there and will benefit from you refactor for #291.

@Fahima-Islam
Copy link
Contributor Author

Fahima-Islam commented Feb 4, 2020 •
edited
Loading

The original issue asked for fitting the first three subruns only. You don't need a whole project file for that. Please create a file with x,y,e for these three spectra (ascii or hdf5) to include in the repository and load that instead.

This would be better located in tests/unit/test_peak_fit_engine.py and re-use functionality that exists there and will benefit from you refactor for #291.

@peterfpeterson Actually I found that this file is already located in /HFIR/HB2B/IPTS-22731/shared/autoreduce/HB2B_1060.h5. I think it is redundancy to add this file as a whole or part of it in the repository. I can just load the file from /HFIR/HB2B/IPTS-22731/shared/autoreduce/HB2B_1060.h5 and work only with first three sub runs for test. What do you think?

@peterfpeterson
Copy link
Member

The data mounts aren't available on the build server and the one in /HFIR/HB2B/IPTS-22731/shared/autoreduce/HB2B_1060.h5 can change without our control. My favorite option is to get the x/y/e values for the first three runs into a "small" file that the test can read in or put those values in a setup function in the the test .py file. The next favorite is to do the work of configuring git-lfs so we can version control the files. After that is including the project file in the git repository directly.

We already have too many tests that don't get run on the build server and end up being frequently broken. Using the data mounts in the pyrs tests will be going away once more urgent work is done.

@Fahima-Islam
Copy link
Contributor Author

@peterfpeterson I reduced the HB2B_1060.nxs.h5 using scripts/reduce_HB2B.py /path/to/HB2B_1060.nxs.h5 --project HB2B_1060.h5. The concern is the reduced file does not have any error. The data set main is only for intensities.
Selection_025
In addition,Some subruns have two peaks But the first three subruns have only one peak which you can see in the 2D plot of the intensities.
Selection_026
Therefore, if you want to verify the model for two peaks, then we have to choose different subruns other than first three subruns.

@peterfpeterson
Copy link
Member

Select 3 subruns that you prefer. The choice of the first 3 was arbitrary.

@peterfpeterson peterfpeterson mentioned this pull request Feb 13, 2020
Closed
@Fahima-Islam Fahima-Islam force-pushed the peak_fitting_unit_test_HB2B_1060 branch from 692ae3b to a74ae40 Compare February 14, 2020 01:50
peterfpeterson
peterfpeterson reviewed Feb 14, 2020
View reviewed changes
Copy link
Member

@peterfpeterson peterfpeterson left a comment

Choose a reason for hiding this comment

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

Why is the new file tests/unit/test_peak_fit_HB2B_1060.py still in this? It looks like the new version has been put in tests/unit/test_peak_fit_engine.py with improvements. Did you forget to delete the old one?

@Fahima-Islam Fahima-Islam force-pushed the peak_fitting_unit_test_HB2B_1060 branch from b03fa70 to b5fab7e Compare February 17, 2020 15:23
@peterfpeterson peterfpeterson merged commit 621f7de into master Feb 21, 2020
@peterfpeterson peterfpeterson deleted the peak_fitting_unit_test_HB2B_1060 branch February 21, 2020 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterfpeterson peterfpeterson peterfpeterson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add peak fitting unit tests from first HB2B_1060
2 participants
@Fahima-Islam @peterfpeterson