Skip to content

Commit 8fd3bc2

Browse files
authored
fix(msstats+): Cover edge case in Linux when the first row of a quality metric has an NA (#142)
1 parent ecb9c1a commit 8fd3bc2

2 files changed

Lines changed: 42 additions & 9 deletions

File tree

inst/tinytest/test_utils_anomaly_score.R

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -383,5 +383,30 @@ duplicate_metrics = run_quality_metrics(
383383
# The last 5 rows (with high values) should have lower mean anomaly scores
384384
# Since they are all clumped between 2 and 4, whereas 0.1 is by itself
385385
expect_true(mean(duplicate_metrics$AnomalyScores[6:10]) < mean(duplicate_metrics$AnomalyScores[1:5]),
386-
info = "Rows 6-10 (values clumped 2-4) should have lower
386+
info = "Rows 6-10 (values clumped 2-4) should have lower
387387
anomaly scores than rows 1-5 (isolated value of 0.1)")
388+
389+
nan_first_row_df = create_base_df(5)
390+
nan_first_row_df$QualityMetric.mean_increase = c(NA, 0.2, 0.4, 0.6, 0.8)
391+
392+
nan_first_row_result = tryCatch({
393+
MSstatsConvert:::.runAnomalyModel(
394+
nan_first_row_df,
395+
n_trees = 100,
396+
max_depth = "auto",
397+
cores = 1,
398+
split_column = "PSM",
399+
quality_metrics = c("QualityMetric.mean_increase"))
400+
}, error = function(e) e)
401+
402+
expect_false(inherits(nan_first_row_result, "error"),
403+
info = paste(
404+
"Anomaly model should not crash/error when a quality metric has a",
405+
"leading NA/NaN value within a PSM group.",
406+
if (inherits(nan_first_row_result, "error"))
407+
paste("Got error:", conditionMessage(nan_first_row_result)) else ""))
408+
409+
if (!inherits(nan_first_row_result, "error")) {
410+
expect_true(all(is.finite(nan_first_row_result$AnomalyScores)),
411+
info = "Anomaly scores should be finite even when a quality metric has a leading missing value")
412+
}

src/isolation_forest.cpp

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -78,22 +78,30 @@ std::unique_ptr<IsolationTreeNode> isolation_tree(
7878
std::string split_feature = features[feature_dist(gen)];
7979

8080
// Can split on numeric or missing value
81-
double min_val = data[0].at(split_feature);
82-
double max_val = min_val;
81+
double min_val = 0.0;
82+
double max_val = 0.0;
8383
bool has_missing = false;
84+
bool has_valid = false;
8485
for (const auto& row : data) {
85-
if (std::isnan(row.at(split_feature))) {
86+
double val = row.at(split_feature);
87+
if (std::isnan(val)) {
8688
has_missing = true;
8789
continue;
8890
}
89-
min_val = std::min(min_val, row.at(split_feature));
90-
max_val = std::max(max_val, row.at(split_feature));
91+
if (!has_valid) {
92+
min_val = val;
93+
max_val = val;
94+
has_valid = true;
95+
} else {
96+
min_val = std::min(min_val, val);
97+
max_val = std::max(max_val, val);
98+
}
9199
}
92-
93-
if (min_val == max_val && !has_missing) {
100+
101+
if (!has_valid || (min_val == max_val && !has_missing)) {
94102
return std::make_unique<IsolationTreeNode>(n);
95103
}
96-
104+
97105
// TODO: Chance to chose missing is 50/50. Could make less likely. Test
98106
bool is_missing_split = false;
99107
double split_value = 0.0;

0 commit comments

Comments
 (0)