Add "bedtools map" with a new group_by logic#17
Conversation
Without this, cargo test fails on macOS with "Library not loaded: libpython3.13.dylib" because the dynamic linker can't find the Python shared library. The build script uses pyo3-build-config (already a build-dependency) to discover the Python lib directory and passes it as an rpath to the linker. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implements bedtools-map-style functionality with composable flags: - Default: aggregate all overlapping B intervals (one row per A) - -G/--group-by-b: group B intervals by name and summarize separately - -n/--name-match: only summarize B intervals whose name matches A - Supports multiple comma-separated columns (-c) and operations (-O) with bedtools-compatible pairing rules (1:N, N:1, N:N) - Operations: count, sum, mean, min, max, median
Summary of ChangesHello @mrvollger, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly extends the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a powerful new map subcommand, which is a great addition. The implementation is well-structured and includes comprehensive command-line help and tests. I've identified a few areas for improvement, mainly concerning portability, API design, error handling, and a logic bug in the name-matching feature. My comments provide specific suggestions to address these points.
… stdout portability - Change compute() to take &[f64] instead of &mut Vec<f64> so Median no longer sorts the caller's data in place; clones to a local vec. - Fix name-match in non-grouped path: B intervals with no name were incorrectly included when A had a name. Now correctly excludes B intervals whose name differs from A's (including when B has no name). - Remove map_err wrappers on write_record/flush/BedWriter::new; BedError implements std::error::Error so ? propagates directly without losing the original error type and context. - Replace /dev/stdout with BedWriter::from_writer(stdout()) for portable stdout handling, and pass PathBuf directly to BedWriter::new to avoid unwrap() on non-UTF-8 paths. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Consolidate inline unit tests and move end-to-end map_command tests to a separate integration test file using the Command-based pattern matching integration_skip_position.rs. Inline tests now cover only pure functions (AggOp, format_number, expand_ops). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…-numeric warnings - Treat missing A name as "." for --name-match (previously silently skipped filtering) - Reject -c 0 with a clear error (columns are 1-indexed, matching bedtools) - Warn once per column on first non-numeric value (matching bedtools behavior) - Add tests for zero overlaps, A-with-no-name + name-match, column 0 error, single-element median Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@codex review and note how we can support python functions in the map sub-command |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b60954f0dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // then emit an rpath so the test/binary can find libpython at runtime on macOS. | ||
| let config = pyo3_build_config::get(); | ||
| if let Some(lib_dir) = &config.lib_dir { | ||
| println!("cargo:rustc-link-arg=-Wl,-rpath,{}", lib_dir); |
There was a problem hiding this comment.
This build script emits cargo:rustc-link-arg=-Wl,-rpath,... for every target whenever lib_dir is present, but that linker syntax is not portable (notably MSVC/Windows), so cross-platform builds can fail at link time even though this rpath workaround is only needed on macOS. Add an explicit target check (for example macOS-only, or at least non-MSVC) before printing this flag.
Useful? React with 👍 / 👎.
| #[arg( | ||
| help = "aggregation operation(s) to apply. Comma-separated for multiple.", | ||
| short = 'O', | ||
| long = "operation", | ||
| default_value = "sum", | ||
| value_delimiter = ',', | ||
| value_enum | ||
| )] | ||
| pub operations: Vec<AggOp>, |
There was a problem hiding this comment.
The map CLI currently restricts -O/--operation to the fixed AggOp enum and never exposes the existing Python-function loading flow used by other commands, so users cannot run custom aggregation logic in map without modifying Rust code. This creates a feature gap for workflows that depend on Python-defined functions; adding a Python-backed operation path (reusing the existing bedder::py compilation/introspection utilities) would make map extensible and consistent with the rest of the tool.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex sketch up and example of what python functions owuld look like here. Update the enum and add other stubs as needed
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
There was a problem hiding this comment.
Codex couldn't complete this request. Try again later.
There was a problem hiding this comment.
@codex don't look for the groupmap branch that's from the person who create the pull request. just checkout the PR
There was a problem hiding this comment.
Codex couldn't complete this request. Try again later.
2 participants
Hi @brentp, per our conversation this morning plus one new feature I always wanted in bedtools map :)
Summary
mapsubcommand that aggregates values from overlapping B intervals for each A interval (likebedtools map)-G/--group-by-b: for each A, group its overlapping B intervals by name, then summarize each group separately (like dplyr'sgroup_by() %>% summarize())-n/--name-match: only summarize B intervals whose name matches A's name-c) and operations (-O) with bedtools-compatible pairing rulesname()andcolumn_as_f64()methods toPositionfor extracting BED fieldsTest plan
format_number(integer vs float output)cargo buildcompiles without warningscargo test— all 90 tests pass