Feature head mapping by AbhirupaGhosh · Pull Request #45 · JRaviLab/amRdata

AbhirupaGhosh · 2026-08-17T18:13:05Z

Description

Introduce feature-dyad mapping in the format of two columns, where dyad is protein|gene combination.

What kind of change(s) are included?

Feature (adds or updates new capabilities)
Bug fix (fixes an issue).
Enhancement (adds functionality).
Breaking change (these changes would cause existing functionality to not work as expected).

Checklist

Please ensure that all boxes are checked before indicating that this pull request is ready for review.

I have read and followed the CONTRIBUTING.md guidelines.
I have searched for existing content to ensure this is not a duplicate.
I have performed a self-review of these additions (including spelling, grammar, and related).
I have added comments to my code to help provide understanding.
I have added a test which covers the code changes found within this PR.
I have deleted all non-relevant text in this pull request template.
Reviewer assignment: Tag a relevant team member to review and approve the changes.

Introduce new function to map features to head instead of protein clusters and change the approach of reading from parquet_duckdb to only parquets.

Some reshuffling but mostly just adding support for manifest and modifying the way parameters are handled to account for manifest detection. Should be able to dynamically add databases through this, but accounting for different column names in those new DBs will take additional effort.

epbrenner

Works for me! But I also added the manifest stuff to this, so somebody else needs to approve before merging.

The merge-base changed after approval.

AbhirupaGhosh · 2026-08-19T23:42:57Z

The integration of manifest JSON in the buildDyadFeatureMap() works as usual.

This PR can be merged.

epbrenner

I still think this PR is cool. I still also think it could use one more review.

The merge-base changed after approval.

…database name column for HMMER generated matrices.

jananiravi

Few quick comments for @AbhirupaGhosh @eboyer221 @epbrenner

jananiravi · 2026-08-27T16:29:07Z

+#' )
+#' }
+#'
+#' @export


renegenerate namespace devtools::document()

jananiravi · 2026-08-27T16:30:00Z

+#'   #' @details
+#' The function performs the following steps:
+#' \enumerate{
+#'   \item Creates a protein-gene mapping from
+#'     \code{genome_gene_protein.parquet}.
+#'   \item Constructs a unique protein-gene dyad identifier of the form
+#'     \code{"protein|gene"}.
+#'   \item Optionally loads structural and HMMER annotations.
+#'   \item Generates a network edge list linking each dyad to its associated
+#'     features.
+#'   \item Writes the resulting edge list to a compressed Parquet file.
+#' }


check rendering in roxygen2 documentation block. fix otherwise

jananiravi · 2026-08-27T16:34:21Z

+        protein_ids AS protein,
+        REPLACE(Gene, '~', '.') AS gene
+      FROM read_parquet('%s')
+      WHERE protein_ids IS NOT NULL


Check: missing empty-string filter, inconsistent with data_processing.R's value != "" convention; can produce bogus "|Gene" dyads.

jananiravi · 2026-08-27T16:39:54Z

+    DBI::dbReadTable(con, count_table) |>
+      tidyr::pivot_longer(
+        -genome_id,
+        names_to = "annotation",
+        values_to = "value"
+      ) |>
+      dplyr::rename(!!database := annotation) |>
+      dplyr::filter(!is.na(value) & value != "") |>
+      dplyr::mutate(value = as.integer(value)) |>
+      writeCompressedParquet(count_parquet)


any new features added here (this PR) that are not part of the PR description -- please go back and edit before merging. thanks!

jananiravi · 2026-08-27T16:42:35Z

+  if ("struct" %in% addtnl_feature_scales) {
+
+    struct_path <- file.path(
+      parquet_dir,
+      "struct.parquet"
+    )
+
+    if (!file.exists(struct_path)) {
+
+      message(
+        "Skipping struct: no parquet found. Generate struct parquet first."
+      )
+


Duplicates the skip-pattern already generalized in create_feature_view() - minor.

jananiravi · 2026-08-27T16:46:40Z

+      fixed = TRUE
+    )
+  }
+


for each short block, worth adding a brief comment on what the succeeding block does. 'brief' would suffice (not just here -- this is true across scripts.

jananiravi · 2026-08-27T16:48:47Z

@@ -0,0 +1,578 @@
+#' Build a protein-gene dyad feature network using DuckDB
+#'
+#' Constructs a bipartite network linking protein-gene dyads to biological


you are talking about many kinds of nodes but are calling it bipartite. 🤔

jananiravi · 2026-08-27T16:51:25Z

+#' protein|gene --> pfam:PFXXXXX
+#' protein|gene --> cog:COGXXXX
+#' protein|gene --> amr:GENE_NAME
+#' protein|gene --> defensecas:FEATURE


make sure the generic FEATURE call doesn't lead to any downstream ambiguity. PD_FEATURE (for phage defense)?

jananiravi · 2026-08-27T16:58:34Z

@AbhirupaGhosh, please update the current PR description with accurate details on what the PR adds/fixes and how to run/check.

Co-authored-by: Janani Ravi <janani.ravi@cuanschutz.edu>

jananiravi · 2026-08-27T16:59:43Z

+#' }
+#'
+#' @export
+buildDyadFeatureMap <- function(


@eboyer221 check if new functions have tests -- if not, add that to your list of bioc to-dos.

jananiravi · 2026-08-27T17:00:25Z

+#' @export
+buildDyadFeatureMap <- function(
+    duckdb_path,
+    addtnl_feature_scales = NULL,


is this used anywhere else? all other parameters seem to be fully spelled out

jananiravi · 2026-08-27T17:01:05Z

+  # Generic HMMER feature view
+  # =========================
+
+  create_feature_view <- function(


internal function starting with . or external with @export? add min. documentation?
@AbhirupaGhosh

jananiravi · 2026-08-27T17:03:01Z

+#'
+#' @export
+buildDyadFeatureMap <- function(
+    duckdb_path,


check for consistency

when is this duckdb_parquet_path vs. duckdb_path?

jananiravi · 2026-08-27T17:07:13Z

+#' )
+#' }
+#'
+#' @export


Missing @import DBI duckdb here — related functions like buildClusterFeatureMap() in R/feature_to_cluster.R declares it for equivalent DBI/duckdb usage. Harmless, but inconsistent.

jananiravi · 2026-08-27T17:08:30Z

+      edge_queries,
+      "
+      SELECT DISTINCT
+        pgd.dyad AS source,


what's pgd.dyad?
protein-gene-dyad.dyad?

…ation - Updated documentation references in `.defenseHMMER`, `.docker_path`, `.id_checker`, `.isValidHmmFile`, `.parseHMMEROutput`, `.parse_hmmer_profiles`, `.pick_shell`, `.prepareHmmerDatabases`, `.runHMMER`, and `.runHmmerJob` to point to the correct source files. - Removed obsolete documentation files for `.getDfIPRColNames` and `.getDfIPRColTypes`. - Added new internal functions for manifest management: `.manifest_event`, `.manifest_file_info`, `.manifest_finish`, `.manifest_git_info`, `.manifest_package_versions`, `.manifest_resume`, `.manifest_run_id`, `.manifest_stage`, and `.manifest_start`. - Introduced `.preserve_export_id_text` helper function to maintain trailing zeroes in exported tables. - Updated `runDataProcessing` and `runPanaroo2Duckdb` documentation to reflect changes in the processing pipeline, including the integration of HMMER. - Enhanced `exportProcessedData` and `exportTables` functions to include new parameters for better control over output formats and table exports. - Removed `.process_chunk` documentation as the function has been deprecated. - Added new function `buildDyadFeatureMap` for constructing a protein-gene dyad feature network.

# Conflicts: # R/helpers.R

…ature_scales, add @import, empty-string filter on protein_gene view, rework comments, add tests

eboyer221 · 2026-08-27T19:23:40Z

Thanks @jananiravi, the requested changes have been addressed in the latest push.

Docs / roxygen

#' #' @details typo fixed : @details now renders as its own section (was being swallowed into output_path). Namespace regenerated with devtools::document().
Added @import DBI duckdb to match buildClusterFeatureMap().
"bipartite" : kept but made precise: it is two-mode (dyad nodes vs. feature nodes, every edge dyad→feature); reworded the description and @details to say so explicitly.
Added DefenseCas to the description (done) and changed the doc placeholder defensecas:FEATURE → defensecas:DEFENSE_SYSTEM; added a note that the type prefix keeps feature namespaces separate so generic names can't collide downstream.
duckdb_path vs duckdb_parquet_path: duckdb_path is the package-wide convention (data_processing.R, data_curation.R, helpers.R, README, vignette) — feature_to_cluster.R is the outlier. Kept duckdb_path here and added a cross-reference to runDataProcessing(). Happy to rename feature_to_cluster.R separately if we want full consistency.
addtnl_feature_scales → additional_feature_scales (spelled out, matches the other params). Updated everywhere + man page.
.create_feature_view() — it's a local closure inside buildDyadFeatureMap(), not a package function; renamed with the . prefix to match the other local helpers (.sql_escape, .parquet_dataset_sql) and added a short doc comment.

Comments

Added brief comments to each block / local helper explaining what the following code does.
pgd.dyad : added a comment: pgd aliases the protein_gene_dyad view, so pgd.dyad is the protein|gene id.

Correctness

Added the empty-string filter (protein_ids <> '', Gene <> '') to the protein_gene view, matching data_processing.R's value != "" convention — prevents bogus protein| / |gene dyads.

Tests

Added tests/testthat/test-buildDyadFeatureMap.R covering the input-validation error paths. Full end-to-end fixture test (small DuckDB + manifest + annotation Parquets) left as a Bioconductor to-do — noted in the file.

Merge conflict with main (R/helpers.R roxygen) resolved.

PR description still needs to be updated.

eboyer221

@AbhirupaGhosh
This looks good to me! I did push a commit to address some of @jananiravi's review comments (roxygen @details rendering, addtnl_feature_scales → additional_feature_scales, @import DBI duckdb, empty-string filter on the protein_gene view, .create_feature_view rename, reworded comments, + a test file for the input-validation paths). Also merged main to clear the conflict that was showing up.

Tested: ran buildDyadFeatureMap() on the Staphylococcus_epidermidis sample Parquets (COG + AMRFinder + struct) and it reproduces the committed dyad_feature.parquet exactly - 483,686 edges, zero row-level diff. load_all()/document() clean, full test suite passes.

Still open: I think the only thing left is just updating the PR description, and a couple of the questions that are calls for you (the PD_/DefenseCas target-prefix naming; whether to rename feature_to_cluster.R's duckdb_parquet_path for consistency).

AbhirupaGhosh · 2026-08-27T19:39:59Z

@eboyer221 One more thing to note is duckdb_path vs duckdb_parquet_path indicates two different database. One contains tables and other contains parquet views.

jananiravi

I'll let Abhirupa go through the most recent changes, too. Looks OK otherwise.

eboyer221 · 2026-08-27T19:54:34Z

@eboyer221 One more thing to note is duckdb_path vs duckdb_parquet_path indicates two different database. One contains tables and other contains parquet views.

Thanks for pointing that out, @AbhirupaGhosh, I hadn't taken note that duckdb_path (tables) and duckdb_parquet_path (views) are genuinely different DBs.

I just look looked at PR #47. It settles the naming question because epbrenner's branch already adds buildDyadFeatureMap(duckdb_path = duckdb_path, ...) into runDataProcessing() (at the end, after cleanData()). So let's keep duckdb_path in #45; renaming to duckdb_parquet_path would just break #47's call site. Functionally it doesn't matter anyway because buildDyadFeatureMap() never opens the DB, it only uses dirname(duckdb_path) to find the sibling parquet files + manifest.

Two things for merge coordination:

Feature head mapping #45 needs to merge before Fix processed data exporter + add more export options #47 (Fix processed data exporter + add more export options #47 calls buildDyadFeatureMap but doesn't add feature_to_head.R).
Non-blocking:
Fix processed data exporter + add more export options #47's new .exportDyadAnnotations() in helpers.R re-implements the sameprotein_gene / protein_gene_dyad views and is missing the empty-string filter Janani flagged on Feature head mapping #45 — worth porting that over (or having it reuse the feature_to_head.R logic) so the dyad construction doesn't drift. The divergence only becomes real once Fix processed data exporter + add more export options #47 rebases onto a merged Feature head mapping #45. That's the natural point for us to either port the <> '' guards into .exportDyadAnnotations() or refactor it to share feature_to_head.R's logic.

AbhirupaGhosh added 4 commits August 14, 2026 17:31

Create feature_to_head.R

f5fec39

Introduce new function to map features to head instead of protein clusters and change the approach of reading from parquet_duckdb to only parquets.

Update feature_to_head.R

c153945

update buildDyadFeatureMap()

396025a

Update the function to check existence of the desired parquet

7981f89

AbhirupaGhosh requested review from eboyer221, epbrenner and jananiravi August 17, 2026 18:13

AbhirupaGhosh self-assigned this Aug 17, 2026

jananiravi mentioned this pull request Aug 17, 2026

Replacing InterProScan with HMMER + .json manifest #44

Merged

epbrenner previously approved these changes Aug 19, 2026

View reviewed changes

AbhirupaGhosh assigned epbrenner Aug 19, 2026

epbrenner previously approved these changes Aug 20, 2026

View reviewed changes

Update data_processing.R to rename the annotation column to specific …

5db1e0f

…database name column for HMMER generated matrices.

jananiravi reviewed Aug 27, 2026

View reviewed changes

Update R/feature_to_head.R

4a584f2

Co-authored-by: Janani Ravi <janani.ravi@cuanschutz.edu>

jananiravi reviewed Aug 27, 2026

View reviewed changes

AbhirupaGhosh and others added 4 commits August 27, 2026 11:34

Documentation

b6ede10

Merge remote-tracking branch 'origin/main' into feature_head_mapping

ad30c6a

# Conflicts: # R/helpers.R

Address PR #45 review: fix roxygen rendering, spell out additional_fe…

c6c0e30

…ature_scales, add @import, empty-string filter on protein_gene view, rework comments, add tests

eboyer221 approved these changes Aug 27, 2026

View reviewed changes

jananiravi approved these changes Aug 27, 2026

View reviewed changes

Uh oh!

Conversation

AbhirupaGhosh commented Aug 17, 2026

Description

What kind of change(s) are included?

Checklist

Uh oh!

epbrenner left a comment

Choose a reason for hiding this comment

Uh oh!

AbhirupaGhosh commented Aug 19, 2026

Uh oh!

epbrenner left a comment

Choose a reason for hiding this comment

Uh oh!

jananiravi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jananiravi commented Aug 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eboyer221 commented Aug 27, 2026

Uh oh!

eboyer221 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AbhirupaGhosh commented Aug 27, 2026

Uh oh!

jananiravi left a comment

Choose a reason for hiding this comment

Uh oh!

eboyer221 commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eboyer221 left a comment •

edited

Loading

eboyer221 commented Aug 27, 2026 •

edited

Loading