add new outlier method - #507
Merged
Merged
Conversation
ESCRI11
requested changes
Aug 6, 2026
ESCRI11
left a comment
ESCRI11
left a comment
Contributor
There was a problem hiding this comment.
-
R/pgx-outlier.R:10 — The default value of
coliscol. Thus R cannot find a value, and every call withplot = TRUEfails. Change the default tocol = "grey70". -
R/pgx-outlier.R:46 — The subset of
Zhas nodrop = FALSE. Thus one method makesZa vector, androwMeans()fails. Adddrop = FALSE.
Contributor
|
Fixing. Will be in the next commit |
- col default was `col=col`, so any plot=TRUE call errored on a missing argument. Default to "grey70", matching plotOutlierScores. - subsetting Z dropped to a vector for single-method calls, breaking rowMeans(). Added drop = FALSE. - gate the isoforest behind "z.isoforest" %in% methods. It ran on every call, including the default where its column is discarded, which made isotree a hard dependency of the default path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- expose outlier_methods on pgx.preprocess() so z.isoforest is reachable. Defaults to the current three methods, not NULL: detectOutlierSamples() reads NULL as "all methods", which would enable the isoforest for every existing caller. - validate methods with match.arg(). An unmatched name silently produced a zero-column Z and NaN z-scores (reading as "no outliers") with plot=FALSE, and died in plotOutlierScores() with "subscript out of bounds" with plot=TRUE. - swap irlba for stdlib svd() in outlier.isoforest_zscore. nv=3 was hardcoded, so it aborted at <=3 samples and warned at 4. Callers cap the input at 1000 rows, so a truncated solver gains nothing. - clamp ndim against ncol(V), the matrix it is applied to, not the input dims. - drop the unused scale argument. - seq_len() instead of 1:ncol(Z). - cover all of the above in test-pgx-outlier.R. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by a second review pass over the outlier changes. - mad() is 0 as soon as over half the values tie, e.g. the same sample uploaded twice. All three z-scores then evaluated 0/0, so z.outlier was NaN and pgx.preprocess died on "missing value where TRUE/FALSE needed". The three methods shared one abs(x-median)/mad expression, so the guard lives in one local zscore(): fall back to the mean absolute deviation, and to zeros only when every value is identical. Returning zeros directly would instead mask a real outlier whenever over half the samples tie. Bit-identical whenever mad() > 0. - make is.outlier NA-safe so no non-finite score can reach that if() again, whatever produces it. - plotOutlierScores: compute ylim from finite values only (non-finite Z aborted with "need finite 'ylim' values", reachable since par gained a working default), and restore the caller's par() on exit. - test the above, plus the tied-sample regression and plot=TRUE on degenerate input. Drop the isoforest which.max assertion: it was seed-luck, and the score's scale and sign are still under review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ESCRI11
approved these changes
Aug 10, 2026
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
2 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.
Add extra 'isolation forest' outlier detection method. Seems to be popular.
PS: Need isotree dependency.