Skip to content

Summarize command - #64

Open
berndbohmeier wants to merge 137 commits into
mainfrom
feature/summary
Open

Summarize command#64
berndbohmeier wants to merge 137 commits into
mainfrom
feature/summary

Conversation

@berndbohmeier

@berndbohmeier berndbohmeier commented Oct 29, 2025

Copy link
Copy Markdown
Collaborator

Edit: This is now ready.
UI:

  • Sample Statistics
    answers which samples are done, which are failing. Gives an idea on how much the sequencing is already done
  • QC Statistics
    gives an idea about how good the sequencing is working. Which experiments have good data, which have high coverage, which are contaminated, etc.
  • Prevalence
    Two ways to show prevalence, grouped by columns in the metadata file

JasonAHendry and others added 29 commits September 16, 2025 09:24
The -e flag is not needed.
Get ready for better text
Ensure we mark duplicates and only take the best sample
Ensure we exclude samples not in master metadata
Make sample_type mandatory. We need it for summary analysis and it is
best if people just include it when creating the sample sheet. It should
not be much extra work.

We maybe want to make it optional if we can derive it from the sample
name, to be discussed.
For now just don't group them by alt alleles. We might have a different
set of alt alleles when we call the same mutation in different
experiments, as we might have a triallelic side. As we in the end group
by amino acid change, this leads to problems. We need to better discuss
how to hanle csq calling correctly for multiple changes.
This helps to only look at the sequenced samples
Green and red should be more intuitve what they mean
It makes sense that we don't report contamination if we have low
coverage, as this could actually just be caused by low coverage. But if
we are over the abs threshold, we can be sure, it's contamination
Exclude columns with to many entries and which are numeric. Maybe we
want to filter more in the future, or have a way to provide a list
They are big and we don't need them for the analysis

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Missing dependencies, destructive output handling, unsupported no-caller experiments, and broken map callbacks prevent reliable use of the new command.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (6)

Previously missed (4) — in code that hasn't changed since the last review.

src/nomadic/summarize/analysis/metadata.py:69

  • Converting IDs with Python str turns a missing value into the literal sample ID "nan". The subsequent validator no longer sees it as null, so blank master-metadata rows become real, not-sequenced samples and can be joined into results. Preserve nullable strings and explicitly reject null or blank IDs during validation.
    src/nomadic/util/summary_settings.py:14
  • With the installed Pydantic 2 API, Optional[...] permits None but does not make a field optional unless it has a default. Thus a valid partial map: block that omits either center or zoom_level raises a validation error despite these fields being modeled as optional.
    src/nomadic/util/summary_settings.py:27
  • An empty or comment-only settings file makes yaml.safe_load return None, and unpacking it with **data raises TypeError. Treat an empty document as the default settings object.
    src/nomadic/summarize/dashboard/components.py:1003
  • The dashboard constructor documents coordinate files as location,latitude,longitude, but this callback indexes lat and long. A file following the documented schema reaches this branch and fails with KeyError; standardize the accepted column names and validate them when loading the file.

src/nomadic/summarize/main.py:121

  • The existing summary is recursively deleted before any experiment, metadata, reference, or VCF validation runs. A typo in an input or a failed external command therefore destroys the last usable summary and leaves a partial replacement. Build in a temporary directory and replace the prior summary only after the new run succeeds (or at minimum defer deletion until validation completes).
    src/nomadic/summarize/dashboard/components.py:962
  • gene and aa_change are not defined in this callback, so selecting the bubble-map style raises UndefinedVariableError/NameError. Filter the analysis rows using the selected target_mutation, as the dropdown value already identifies both fields.
  • Files reviewed: 202/213 changed files
  • Comments generated: 4
  • Review effort level: Balanced

Comment thread setup.cfg
Comment thread src/nomadic/summarize/main.py
Comment thread src/nomadic/summarize/main.py Outdated
Comment thread src/nomadic/summarize/dashboard/components.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Settings loading, environment dependencies, and destructive output replacement introduce concrete runtime and reliability failures.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

environments/dev.yml:20

  • The development environment omits the newly required pydantic dependency, while CI installs the package with --no-deps. A clean development/CI environment can therefore fail when the summarize implementation imports summary_settings; keep this environment aligned with the runtime and conda package dependency lists.
  - statsmodels
  - seaborn

src/nomadic/summarize/main.py:279

  • This output name contains a comma instead of the separator used by every other VCF artifact, producing summary.variants.annotated,vcf.gz. Use the standard .annotated.vcf.gz name so downstream tooling and users can identify it consistently.
    src/nomadic/util/summary_settings.py:14
  • In Pydantic v2, an Optional[...] annotation still defines a required field unless it has a default. As written, map: {shape_name_key: ...} (or a map block that supplies only one display option) raises a validation error even though both values are modeled and consumed as optional. Give both fields None defaults.

This issue also appears on line 25 of the same file.

src/nomadic/util/summary_settings.py:27

  • An empty or comment-only settings file makes yaml.safe_load return None, so unpacking it with Settings(**data) raises TypeError. Since every top-level setting has a default, such a file should load as the default settings object.
    src/nomadic/summarize/main.py:122
  • The command deletes the existing valid summary before validating the experiments or completing the replacement. Any later input, reference, bcftools, or analysis failure therefore destroys the last usable summary and leaves only a partial output. Build in a sibling temporary directory and atomically replace the old summary only after successful completion.
  • Files reviewed: 202/213 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

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.

4 participants