Fix sometimes-failing LARS tests #3701
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
conradsnicta
approved these changes
May 14, 2024
shrit
approved these changes
May 14, 2024
Comment on lines
-11
to
-14
|
|
||
| // Note: We don't use BOOST_REQUIRE_CLOSE in the code below because we need | ||
| // to use FPC_WEAK, and it's not at all intuitive how to do that. | ||
|
|
| typedef typename MatType::elem_type ElemType; | ||
|
|
||
| const ElemType tol = (std::is_same<ElemType, double>::value) ? 1e-5 : 1e-3; | ||
| const ElemType tol = (std::is_same<ElemType, double>::value) ? 1e-5 : 5e-3; |
Member
There was a problem hiding this comment.
This is a nice way to make the usage of std::is_same in a condition with out the need for constexpr
This was referenced May 26, 2024
Closed
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed that the tests for LARS were failing more than once every hundred trials with different random seeds, so I started digging in to see what was wrong. I came out with a few conclusions:
With
floatas an element type, thePredictTestcan be more flaky---this is expected due to less precision. So now it allows multiple trials when the element type isfloat.When a singularity is encountered, we try to remove the most-recently-added variable from the active set. However, the
activeGramMatrixwas not removing its columns correctly.When computing the signs of correlations, if the correlation was exactly 0, then there was a division by 0. I fixed that just by adding a small value to the denominator.
The check to see if an externally-passed Gram matrix satisfied the expected conditions of mean-centeredness and unit-variance was more trouble than it was worth, and I think the note in the documentation is good enough. So I removed the check, since it does not seem to be reliable.
Then, I ran
[LARSTest]1000 times with different random seeds and encountered no failures, so, hopefully this should help with test robustness.