Skip to content

hifiasm: fix --primary output, --hg-size decimals, test typos, modern profile#7985

Open
alpapan wants to merge 3 commits into
galaxyproject:mainfrom
alpapan:hifiasm-bugfixes
Open

hifiasm: fix --primary output, --hg-size decimals, test typos, modern profile#7985
alpapan wants to merge 3 commits into
galaxyproject:mainfrom
alpapan:hifiasm-bugfixes

Conversation

@alpapan
Copy link
Copy Markdown

@alpapan alpapan commented May 13, 2026

Summary

A handful of bug fixes and test/profile cleanup in tools/hifiasm/hifiasm.xml.

Bug fixes

  • primary_contig_graph disappears when the --primary checkbox is set. Three <data> elements share the name primary_contig_graph; Galaxy only registers the last, whose filter requires the opposite checkbox state. The Primary-off primary contig output is now named bp_primary_contig_graph, and the hap1_contigs_std / hap2_contigs_std labels are suffixed with "(balanced haplotype)" to disambiguate from the trio outputs (resolves a OutputsLabelDuplicatedFilter warning).
  • --primary toggle was a no-op. The command section had an unconditional --primary and a broken nested #if (the 'blank' branch was inside the 'set' branch and could never fire). Reorganised so the user-facing checkbox actually controls the flag while preserving "primary on" as the default.
  • --hg-size silently drops decimal points. Entering 1.3g was sanitised to 13g. Added . to the sanitizer's valid set and tightened the validator regex to ^[0-9]+(\.[0-9]+)?[kKmMGg]?$. Help text updated.

Tests / lint

  • Wrap <param name="reads"> + <param name="mode_selector"> inside <conditional name="mode"> for the standard / ONT tests, so they validate under modern Galaxy tool profile versions (resolves 15× TestsCaseValidation).
  • Test 5 (trio): reads moved to trio level; trio_input_selector moved inside trioinput to match the actual input structure.
  • Test 9: max_koocmax_kocc (resolves TestsParamInInputs ERROR).
  • Test 11: hom-covhom_cov (param name is derived from argument="--hom-cov", hyphens become underscores).
  • New Test 17: exercises the --primary OFF path (balanced haplotype outputs).
  • Add profile="23.0" (was implicitly legacy 16.01) and bump VERSION_SUFFIX to 4.

Test plan

  • planemo lint — clean (no warnings, no errors).
  • planemo test against existing test data.
  • Manual: set --hg-size to 1.3g and confirm the command line carries 1.3g, not 13g.
  • Manual: enable Assembly options + Primary checkbox; confirm primary contig graph appears in history.

alpapan added 2 commits May 13, 2026 16:55
… profile

* Fix duplicate output names causing primary_contig_graph to disappear
  when --primary is enabled. The output was declared multiple times under
  the same name, so Galaxy registered only the last (filter-disabled) one.
  The "Primary off mode" primary contig output is now named
  bp_primary_contig_graph; hap1/hap2 std labels are suffixed
  "(balanced haplotype)" to disambiguate from the trio outputs.
* Wire the --primary toggle correctly: drop the unconditional --primary
  in the command line and fix the broken nested cheetah #if so the
  user-facing checkbox actually controls the flag.
* --hg-size: accept decimal values (e.g. 1.3g). The sanitizer was
  stripping the dot, turning 1.3g into 13g silently. Added "." to the
  valid character set and tightened the validator regex.
* Tests: wrap reads/mode_selector in <conditional name="mode"> so tests
  validate under modern Galaxy tool profiles; fix max_kooc -> max_kocc
  (Test 9) and hom-cov -> hom_cov (Test 11) typos; fix Test 5 trio
  param placement (reads at trio level, trio_input_selector inside
  trioinput).
* Add profile="23.0" (was implicitly the legacy 16.01 profile) and bump
  VERSION_SUFFIX to 4.
Comment thread tools/hifiasm/hifiasm.xml Outdated
Copy link
Copy Markdown
Member

@RZ9082 RZ9082 left a comment

Choose a reason for hiding this comment

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

Could you also please remove the .lint_skip file, and fix the linting errors if there are any?

Comment thread tools/hifiasm/hifiasm.xml Outdated
Comment thread tools/hifiasm/hifiasm.xml
Comment thread tools/hifiasm/hifiasm.xml Outdated
Comment thread tools/hifiasm/hifiasm.xml Outdated
* Use @Profile@ token (25.0) instead of hardcoded profile attr.
* Replace `#if $assembly_options.primary: --primary #end if` with
  direct `$assembly_options.primary` substitution (canonical Galaxy
  idiom for booleans with truevalue/falsevalue).
* Switch `name="primary"` to `argument="--primary"` so the param
  identifier auto-derives from the CLI flag; help text trimmed.
* Fix test 17: `expect_num_outputs` 5 -> 6 (the noseq_files
  collection is counted), and switch the hap2 content assertion
  from `^S` regex to `has_size value="0"` since hifiasm emits an
  empty hap2 GFA for the tiny hifiasm-in1.fa.gz fixture.
* Remove tools/hifiasm/.lint_skip; planemo lint is now clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread tools/hifiasm/hifiasm.xml
</output>
<output name="hap2_contigs_std" ftype="gfa1">
<assert_contents>
<has_size value="0"/>
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 does not look correct

Comment thread tools/hifiasm/hifiasm.xml
</assert_command>
<output name="bp_primary_contig_graph" ftype="gfa1">
<assert_contents>
<has_text_matching expression="^S"/>
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 there just nothing included? Or why is this test so flax/loose?

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.

3 participants