Compare the signs of the two rho z scores - #286
Open
arpitjain099 wants to merge 1 commit into
Open
Conversation
The rho outlier rule is written as sign(.data$z_score_rho != sign(.data$z_score_rho_t_plus_1)) The close parenthesis is one term too far right, so instead of comparing the two signs it compares a z score against the sign of the next one and takes the sign of that logical. sign(TRUE) is 1, so the term is true whenever z_score_rho is not exactly 1 or -1, which makes the sign test pass for almost any pair. The effect is that two consecutive steep moves in the SAME direction satisfy the rule, so a sustained rise or fall gets flagged as an outlier and can then be excluded from the model. Only the spike case, a steep move followed by a steep move the other way, was meant to match. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #279.
The rho rule in
flag_ww_outliers()readsThe close parenthesis is one term too far to the right. Instead of comparing the two signs, it compares a z score against the sign of the next one and then takes the sign of that logical.
sign(TRUE)is1, so the term is true wheneverz_score_rhois anything other than exactly1or-1, which is almost always.That makes the sign test a no-op in practice, and the rule reduces to "two consecutive steep moves", regardless of direction. A sustained rise or fall then matches, and gets flagged as an outlier, which
indicate_ww_exclusions()can go on to exclude from the model. Only the spike case, a steep move followed by a steep move the other way, was meant to match.Running the same data through both versions, with
log_conc_thresholdset high so only the rho rule can fire:So the change removes the false positives and keeps the true one.
Two tests added, at three thresholds each. One asserts a monotone rise and a monotone fall produce no outlier, and the other asserts the spike still produces exactly one, so the fix cannot pass by simply flagging less. The first fails on
mainwith six failures; the second passes on both, which is the point of including it.All the pre-existing tests in
test_flag_as_ww_outliers.Rpass against both versions, so nothing that was flagged before and should still be flagged has changed. I ran them by sourcingR/preprocessing.Rdirectly, since the suite'ssetup.Rloads the compiled package and I do not have the Stan toolchain here. CI covers the rest.