Skip to content

Also it should be converted to a series of ":=" I think #210

Description

@Rudhik1904

Also it should be converted to a series of ":=" I think

Originally posted by @mstaniak in #208 (comment)

I looked into this but think the current input[, list(...)] is actually the right call here, for two reasons:
  1. In-place := on input would corrupt the caller. .selectHighQualityFeatures is called from MSstatsSelectFeatures, which reuses the same input immediately afterwards:

features_quality = .selectHighQualityFeatures(input, min_feature_count)
input = merge(input, features_quality,
by.x = c("LABEL", "PROTEIN", "FEATURE", "originalRUN"),
by.y = c("label", "protein", "feature", "run"))
Since data.table :=/setnames modify by reference across function boundaries, renaming the columns to lowercase (or adding log2inty/is_obs) inside the function would mutate the caller's table and break that merge (its by.x keys would no longer exist). The input[, list(...)] form avoids this by returning a new table and only rebinding the local name.

  1. It's already a minimal projection, not a full-table copy. list(...) materializes only the 7 columns the feature-selection step needs — it's not the kind of whole-table duplication this PR is targeting.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions