Add find_zero function #8
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
📢 Thoughts on this report? Let us know! |
test/runtests.jl
Outdated
| # Test case 2: Using optimization method | ||
| target_mean = 1.0 | ||
| expected_coefficient = 0.0 | ||
| @test fit_profile(profile_values, target_mean) ≈ expected_coefficient atol = 1e-3 |
There was a problem hiding this comment.
I understand what you're doing, and the code seems correct, but I don't know what you want to do, so I am not sure it this is the best approach.
This solution is to change all profile values to 1. Is that what you want in this situation?
Are the profiles always between 0.0 and 1.0? Is that why you use p^x?
There was a problem hiding this comment.
I understand what you're doing, and the code seems correct, but I don't know what you want to do, so I am not sure it this is the best approach. This solution is to change all profile values to 1. Is that what you want in this situation? Are the profiles always between 0.0 and 1.0? Is that why you use p^x?
The idea is to test that part of the code that uses the optimization procedure. My problem is that I couldn't develop a naive set of values that don't use the find_zero function. So, I forced the use by just going for a target I knew would fail, even if the solution was trivial. I'm open to ideas to test the function better 😄
There was a problem hiding this comment.
The test is fine. I am just not sure if this is what you want for your scaler because to me it doesn't make sense to "scale" all values to 1.0.
There was a problem hiding this comment.
I've updated the tests to make them more meaningful in our expected results. If the input data is in per unit ([0,1]) then you need either one method (find_zero) or the other (optimization) depending on the target value. If the target value is greater than the current mean, it uses find_zero, whereas if it is below, it uses the optimization. You can double-check in the new test the values that I proposed. Nevertheless, it is fair to say that most of our profiles will increase the mean, so we will mainly use the find_zero part.
| function test_fit_profile() | ||
|
|
||
| # Test case 1: Using root finding method | ||
| profile_values = [0.2, 0.5, 0.9, 1.0, 0.4, 0.1, 0.0] | ||
| target_mean = 0.5 | ||
| coefficient = fit_profile(profile_values, target_mean) | ||
| @test coefficient ≈ 0.7491921789832345 atol = 0.01 | ||
| expected_coefficient = 0.7491921797062476 | ||
| @test fit_profile(profile_values, target_mean) ≈ expected_coefficient atol = 1e-3 | ||
|
|
||
| # Test case 2: Using optimization method | ||
| target_mean = 0.4 | ||
| expected_coefficient = 1.2550552057968192 | ||
| @test fit_profile(profile_values, target_mean) ≈ expected_coefficient atol = 1e-3 | ||
| end |
There was a problem hiding this comment.
It makes sense to move this inside the @testset. The scope will also be limited.
* Add find_zero function * Update the test cases * Update fit_profile function documentation
Pull request details
Describe the changes made in this pull request
Changes to add the find_zero function to the fitting function
List of related issues or pull requests
Closes #4
Collaboration confirmation
As a contributor I confirm