Skip to content

Export metadata and feature data tables - #34

Open
epbrenner wants to merge 8 commits into
mainfrom
export_tables
Open

Export metadata and feature data tables#34
epbrenner wants to merge 8 commits into
mainfrom
export_tables

Conversation

@epbrenner

Copy link
Copy Markdown
Contributor

Testing:

For this PR, I have included bundled test data in /inst/extdata/Campy_testdata.zip. Extract this into a single directory of your choosing. This bundled data is not intended to the ultimate example data for final package release.

For data_curation.R:
Run
prepareGenomes("Staphylococcus argenteus")
then
exportTables("data/Staphylococcus_argenteus/Sar.duckdb")

For data_processing.R:
Unzip the Campy test data first, then run
exportProcessedData("inst/extdata/Campy_testdata/Cco_parquet.duckdb")

There are several thrilling parameters to fiddle around with.

What this does:

Adds the ability to export basic stats into human readable CSVs from data_curation.R workflow.
Adds the ability to export all the processed feature data into a variety of formats from data_processing.R workflow.

What's the point?

Makes amRdata more independently useful to people even if they don't run the full amR suite. For example, you could export feature matrices for custom modeling workflows outside of ours, or just get an idea of what distributions of different QC and AST labels are across different taxa.

In data_curation.R, adding ability to export genome stats and QC metadata into human readable CSVs. `load_tables` allows you to run "example <- exportTables("data/Staphylococcus_aureus/Sau.duckdb", load_tables=TRUE)" and import the tables into R so you can play around with them directly. By default, it just saves the tables as exported CSVs. In data_processing.R, added a similar exporter that allows you to save out all the feature data and other DuckDB-connected tables, with the ability to output sequences too, and append AST phenotype data to the feature tables. Tables can be saved out as csv (default), tsv, Parquet, and xlsx. Added `writexl` to Suggests in DESCRIPTION. Fixed some reversions where I broke things in the PR this branch is based on. Will add those same fixes to that PR momentarily to keep things aligned.
Ran data processing workflow for Campylobacter coli. This can be unzipped and used as the test data for exportProcessedData() rather than having to run it all beforehand. Data curation step still requires that prepareGenomes() be run for a test bug, and Staphylococcus argenteus is a good use case for this.
@epbrenner epbrenner self-assigned this Jul 28, 2026
@epbrenner epbrenner added the enhancement New feature or request label Jul 28, 2026

@jananiravi jananiravi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few quick comments. Will test it out tomorrow.

Comment thread R/data_curation.R
Comment thread R/data_curation.R Outdated
Comment thread R/data_curation.R Outdated
Comment thread R/data_curation.R Outdated
Comment thread R/data_curation.R Outdated
Comment thread R/data_curation.R Outdated
Comment thread R/data_curation.R Outdated
Comment thread R/data_curation.R Outdated
Clarified a few names based on code review feedback.
jananiravi
jananiravi previously approved these changes Jul 29, 2026
@jananiravi

Copy link
Copy Markdown
Member

Few quick comments. Will test it out tomorrow.

@epbrenner prepareGenomes + exportTables --> they run locally with Sar.

@jananiravi

jananiravi commented Jul 29, 2026

Copy link
Copy Markdown
Member

After running the campy --> metadata.csv

image

what's TET doing in resistant_classes? genome_drug.resistance_phenotype marks them as susceptible. @epbrenner @AbhirupaGhosh

@AbhirupaGhosh

Copy link
Copy Markdown
Contributor

After running the campy --> metadata.csv

image what's TET doing in `resistant_classes`? genome_drug.resistance_phenotype marks them as susceptible. @epbrenner @AbhirupaGhosh

resistant_classes is a column that adds all the drug classes the genome is resistant to. If there are more than 1, say TET and FLQ, it will show TET_FLQ and it shows susceptible to the genomes that are resistant to none.

@jananiravi jananiravi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this going to be merged to qc_filtering and then qc_filtering --> main? that might be helpful to avoid duplicate reviews.

Comment thread R/data_processing.R
Comment thread R/data_curation.R Outdated
Comment on lines +2486 to +2493
count_if_present("metadata"),
count_if_present("genome_data"),
count_if_present("amr_phenotype"),
count_if_present("metadata_qc"),
count_if_present("metadata_qc_rejections"),
count_if_present("filtered"),
count_if_present("files"),
count_if_present("bac_data")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do these checks tie back to basic_tables? is the latter not exported? so should "files" be added to basic_tables?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored to use a default_tables list rather than duplicating this way. There was logic for the checks, but upon careful review, the logic belonged in the garbage.

Comment thread R/data_curation.R Outdated

@eboyer221 eboyer221 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I tested this, it looks like genome IDs such as 263.1, 263.10, and 263.100 are being written to the CSV as plain numbers rather than text, since they happen to look numeric. When I read that export back (with readr::read_csv(), pandas, or Excel's defaults) all three collapsed to the same value, since trailing zeros after a decimal don't count (263.1 and 263.10 parse as numerically identical). So the three distinct genomes merged under one ID, and it'll happen for any taxon with more than ~9 genomes. I also tried forcing quotes around every value (quote = "all") to see if that would prevent it, but the collision still happened on read. Quoting only protects against commas breaking the file format, it doesn't tell the reader "treat this as text."

This affects every genome_id-keyed table both export functions produce (genome_data, metadata, amr_phenotype_wide, gene_count, etc.).
Two options I see: (1) add a clear warning telling users to force col_types/dtype=str on ID columns when reading CSV/TSV back, or (2) recommend Parquet as the default/safe export format for programmatic reuse, since arrow preserves the real column type - I verified this did not occur with Parquet.

Comment thread R/data_curation.R Outdated

@epbrenner epbrenner left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I have to submit my own review to my own PR to actually post these comments, huh? Oops.

Comment thread R/data_curation.R
Comment thread R/data_curation.R Outdated
Comment thread R/data_curation.R Outdated
Comment thread R/data_curation.R Outdated
Comment thread R/data_curation.R Outdated
Comment thread R/data_curation.R Outdated
Comment thread R/data_curation.R Outdated
@jananiravi

Copy link
Copy Markdown
Member

When I tested this, it looks like genome IDs such as 263.1, 263.10, and 263.100 are being written to the CSV as plain numbers rather than text, since they happen to look numeric. When I read that export back (with readr::read_csv(), pandas, or Excel's defaults) all three collapsed to the same value, since trailing zeros after a decimal don't count (263.1 and 263.10 parse as numerically identical). So the three distinct genomes merged under one ID, and it'll happen for any taxon with more than ~9 genomes. I also tried forcing quotes around every value (quote = "all") to see if that would prevent it, but the collision still happened on read. Quoting only protects against commas breaking the file format, it doesn't tell the reader "treat this as text."

This affects every genome_id-keyed table both export functions produce (genome_data, metadata, amr_phenotype_wide, gene_count, etc.).
Two options I see: (1) add a clear warning telling users to force col_types/dtype=str on ID columns when reading CSV/TSV back, or (2) recommend Parquet as the default/safe export format for programmatic reuse, since arrow preserves the real column type - I verified this did not occur with Parquet.

@epbrenner @AbhirupaGhosh didnt we have this fix in already?

epbrenner and others added 2 commits July 31, 2026 13:55
Co-authored-by: Emily Boyer <130874527+eboyer221@users.noreply.github.com>
@epbrenner

Copy link
Copy Markdown
Contributor Author

When I tested this, it looks like genome IDs such as 263.1, 263.10, and 263.100 are being written to the CSV as plain numbers rather than text, since they happen to look numeric. When I read that export back (with readr::read_csv(), pandas, or Excel's defaults) all three collapsed to the same value, since trailing zeros after a decimal don't count (263.1 and 263.10 parse as numerically identical). So the three distinct genomes merged under one ID, and it'll happen for any taxon with more than ~9 genomes. I also tried forcing quotes around every value (quote = "all") to see if that would prevent it, but the collision still happened on read. Quoting only protects against commas breaking the file format, it doesn't tell the reader "treat this as text."
This affects every genome_id-keyed table both export functions produce (genome_data, metadata, amr_phenotype_wide, gene_count, etc.).
Two options I see: (1) add a clear warning telling users to force col_types/dtype=str on ID columns when reading CSV/TSV back, or (2) recommend Parquet as the default/safe export format for programmatic reuse, since arrow preserves the real column type - I verified this did not occur with Parquet.

@epbrenner @AbhirupaGhosh didnt we have this fix in already?

Yeah, relatively easy solution, just requires special handling of the columns of interest. Patched in the latest commit.

@epbrenner
epbrenner requested a review from eboyer221 July 31, 2026 22:18

@jananiravi jananiravi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor edits, comments added. yet to run line-by-line; that's the reason I didn't approve yet, but looks good.

Comment thread R/data_processing.R
Comment on lines +1966 to +1968
genome_id = as.character(`genome.genome_id`),
antibiotic = as.character(`genome_drug.antibiotic`),
phenotype = as.character(`genome_drug.resistant_phenotype`)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need this special treatment for anything else? accessions that end in a.x as well? (all of them?)

Comment thread R/data_curation.R
exportTables <- function(duckdb_path,
output_dir = NULL,
tables = NULL,
skip_tables = c(NULL),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
skip_tables = c(NULL),
skip_tables = NULL,

Comment thread R/data_curation.R
stop("No tables left to export after applying skip_tables.")
}

preserve_id_text <- function(df) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the two preserve_id_text function definitions aren't identical. Can you check why there are two and if they match? One would be future-proof.

Comment thread R/data_processing.R
}

# Save trailing zeroes
preserve_id_text <- function(df) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more present!

Comment thread R/data_processing.R
"-o", output_dir_cont,
"--clean-mode", "strict",
"--merge_paralogs",
"--refind-mode off",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will eventually inherit the many off-related changes downstream, I suppose. 🤔

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants