Skip to content

Comments

Add ggoncoplot genomic landscape visualization module#112

Merged
sbalci merged 1 commit intomasterfrom
claude/issue-106-20250711_154053
Jul 11, 2025
Merged

Add ggoncoplot genomic landscape visualization module#112
sbalci merged 1 commit intomasterfrom
claude/issue-106-20250711_154053

Conversation

@sbalci
Copy link
Owner

@sbalci sbalci commented Jul 11, 2025

Implements ggoncoplot package features for genomic landscape visualization

Summary

  • Add new jamovi module for creating oncoplots (mutation landscapes)
  • Support for classic oncoplots, gene frequency plots, and co-occurrence analysis
  • Clinical annotation support and customizable visualizations

Implementation

  • Added ggoncoplot dependency to DESCRIPTION and remotes
  • Created complete jamovi 4-file module architecture
  • Integrated with ClinicoPath menu under "Molecular Pathology Plots"

Files Modified/Created

  • DESCRIPTION: Added dependency
  • jamovi/0000.yaml: Module configuration
  • jamovi/ggoncoplot.*: Analysis, UI, and results definitions
  • R/ggoncoplot.b.R: Backend implementation

Resolves #106

Generated with Claude Code

- Add ggoncoplot package dependency to DESCRIPTION and remotes
- Create new jamovi module for genomic landscape visualization
- Implement classic oncoplots, gene frequency plots, and co-occurrence analysis
- Add comprehensive UI with gene/sample selection and clinical annotations
- Support for mutation data visualization with customizable color schemes
- Place module under new ''Molecular Pathology Plots'' menu group

Co-authored-by: Serdar Balcı <sbalci@users.noreply.github.com>
@claude
Copy link
Contributor

claude bot commented Jul 11, 2025

Claude finished @sbalci's task —— View job


PR Review: Add ggoncoplot genomic landscape visualization module

I've completed a comprehensive review of the new ggoncoplot module implementation. Here's my detailed feedback:

Review Checklist

  • Read and understand CLAUDE.md guidelines
  • Review DESCRIPTION file changes
  • Review jamovi module configuration (0000.yaml)
  • Review analysis definition (ggoncoplot.a.yaml)
  • Review UI definition (ggoncoplot.u.yaml)
  • Review results definition (ggoncoplot.r.yaml)
  • Review backend implementation (ggoncoplot.b.R)
  • Check adherence to jamovi 4-file architecture
  • Evaluate code quality and best practices
  • Identify potential bugs and issues
  • Assess performance considerations
  • Check security concerns
  • Evaluate test coverage
  • Provide comprehensive feedback

🎯 Summary

Overall Assessment: Good foundation with proper jamovi architecture, but several code quality issues need attention before merging.

✅ Strengths

Architecture & Integration

  • Perfect jamovi 4-file structure: Follows CLAUDE.md guidelines exactly (.a.yaml, .u.yaml, .r.yaml, .b.R)
  • Proper namespace integration: Correctly uses ClinicoPath namespace
  • Menu placement: Well-organized under "Molecular Pathology Plots" subgroup
  • Dependency management: Correctly added ggoncoplot package to DESCRIPTION and Remotes

User Interface Design

  • Logical UI grouping: Controls well-organized in "Plot Configuration", "Display Options", "Plot Appearance"
  • Comprehensive options: Good coverage of oncoplot customization features
  • Proper variable types: Appropriate suggestions and permissions for variables

Results Structure

  • Multiple output tables: Comprehensive results including mutation summary, sample summary, clinical annotations
  • Conditional visibility: Tables show/hide appropriately based on options
  • Proper formatting: Column types and formats are appropriate

⚠️ Critical Issues to Address

1. Dependency Usage Problem (R/ggoncoplot.b.R:8)

#' @import ggoncoplot

Issue: The code imports ggoncoplot but doesn't actually use it for plot creation. Instead, it reimplements oncoplot functionality with base ggplot2.

Fix: Either use ggoncoplot's functions (recommended) or remove the dependency.

2. Unsafe Variable Reference (R/ggoncoplot.b.R:105)

df <- df %>% dplyr::arrange(!!sym(sampleVar))

Issue: Using !!sym() can be unsafe. Should use all_of() or similar.

Fix:

df <- df %>% dplyr::arrange(.data[[sampleVar]])

3. Incomplete Clinical Data Processing (R/ggoncoplot.b.R:295-303)

clinical_summary <- data.frame(
    variable = self$options$clinicalVars,
    categories = "Various",  # Hardcoded!
    missing = 0,             # Not calculated!
    complete = nrow(prepared_data$data)  # Wrong calculation!
)

Issue: Clinical summary table has hardcoded/incorrect values.

4. Inefficient Co-occurrence Calculation (R/ggoncoplot.b.R:199-209)

for (i in 1:length(genes)) {
    for (j in 1:length(genes)) {
        if (i != j) {
            cooccurrence_matrix[i, j] <- cor(data[[genes[i]]], data[[genes[j]]], use = "complete.obs")
        }
    }
}

Issue: Nested loops when cor() can handle matrices directly.

Fix:

gene_data <- data[genes]
cooccurrence_matrix <- cor(gene_data, use = "complete.obs")

5. Missing Error Handling (R/ggoncoplot.b.R:84-86)

df[[var]] <- as.numeric(as.character(df[[var]]))
df[[var]][is.na(df[[var]])] <- 0
df[[var]] <- ifelse(df[[var]] > 0, 1, 0)

Issue: No validation that conversion to numeric succeeded or handling of non-numeric data.

6. Inconsistent Table Population (R/ggoncoplot.b.R:285-287)

total_mutations = mutation_count,
genes_mutated = mutation_count,  # Same value for different concepts

Issue: genes_mutated should count distinct mutated genes, not total mutations.

🚀 Recommendations for Improvement

Code Quality

  1. Use the ggoncoplot package properly - Leverage its main functions instead of reimplementing
  2. Add comprehensive error handling for data validation and type conversion
  3. Implement proper clinical data analysis instead of placeholder values
  4. Optimize matrix operations using vectorized functions
  5. Add input validation for edge cases (empty data, all zeros, etc.)

Performance

  1. Replace nested loops with vectorized operations where possible
  2. Consider data size limits and add warnings for very large datasets
  3. Cache expensive calculations if plots are regenerated frequently

Security & Best Practices

  1. Sanitize variable names before using in dynamic contexts
  2. Use safe dplyr patterns like .data[[var]] instead of !!sym()
  3. Add bounds checking for numeric parameters

Testing

  1. No tests provided - Critical gap for a complex visualization module
  2. Recommend adding unit tests for data preparation functions
  3. Add integration tests with sample genomic data

Documentation

  1. Add more detailed parameter descriptions in .a.yaml
  2. Include data format examples in help text
  3. Document expected data structure more clearly

🎯 Next Steps

Before merging, please address:

  1. Fix the 6 critical issues identified above
  2. Either properly use ggoncoplot package functions or remove the dependency
  3. Add comprehensive error handling and input validation
  4. Add at least basic unit tests

Nice to have for future iterations:

  1. Support for more mutation types beyond 0/1
  2. Integration with standard genomic data formats (MAF, VCF)
  3. Advanced statistical tests for co-occurrence analysis
  4. Export functionality for plots and tables

This is a solid foundation for genomic visualization in ClinicoPath, but needs refinement before production use.


@sbalci sbalci merged commit 4a6fa19 into master Jul 11, 2025
2 of 13 checks passed
@sbalci sbalci deleted the claude/issue-106-20250711_154053 branch July 11, 2025 16:01
sbalci added a commit that referenced this pull request Nov 3, 2025
Add ggoncoplot genomic landscape visualization module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ggoncoplot

1 participant