Internal strain-genomic-segment record class (model side) - #209
Open
jbrestel wants to merge 42 commits into
Open
Internal strain-genomic-segment record class (model side)#209jbrestel wants to merge 42 commits into
jbrestel wants to merge 42 commits into
Conversation
Introduces the design for a new internal `strain-genomic-segment` WDK record class that translates reference-coordinate genomic segments into isolate/strain coordinates using apidb.indel, terminating at a BED reporter. Key decisions recorded: - Primary key carries reference coordinates plus strain; strain coordinates are derived in an attribute query, preserving provenance and picking up indel reloads without recomputation. - Strain vocabulary comes from apidb.indel joined through study.protocolappnode (there is no strain column on apidb.indel); scoped per organism. - Queries against partitioned webready.*_p tables must constrain org_abbrev. - BED chrom must equal <strain>_<refSeq>, the dnaseq consensus FASTA index key; the name column is free and becomes the output defline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The spec flagged "ontology absence hides the search" as an unverified assumption. Verified against Model/lib/wdk/ontology/individuals.txt and recorded the result as a new section 6.1. Two mechanisms exist for hiding a search. Omitting the row is correct here, matching DynSpansBySegIds and DynSpansByLocation, which are both absent from individuals.txt while remaining addressable through the service API. The "internal webservice" scope combination is used only by injected per-dataset searches, which need ontology presence for categorization. Consequence: this work adds no ontology node, so wb model suffices and wb ontology is not required. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eight tasks, each independently committable, covering the grammar class with unit tests, the strain vocabulary param, the validating ID query, the coordinate-conversion attribute query, the record class and model imports, the BED reporter, end-to-end verification, and a UniDB scope fix on the existing eQTL segment search. Two findings from writing it that shaped the tasks: - The attribute query cannot use webready.*_p at all. Attribute queries receive only primary key columns, so there is no org_abbrev to prune partitions with, and every index on those tables is org_abbrev-prefixed. There is also no unpartitioned GenomicSeqAttributes in this database, which is why DynSpan's Bfmv query cannot run here. The plan routes through dots.ExternalNaSequence instead, which is unpartitioned and indexed on source_id. - Tasks 2 through 4 land model XML before it is imported, so each commits safely without a working model. Task 5 adds the imports and is the first task that can fail a build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The new record works on every project, so it carries no includeProjects or excludeProjects anywhere: not on the record class, the question, the queries, or the primary key columns. This removes an excludeProjects="UniDB" on the project_id PK column and the associated UniDB query variants, both carried over from DynSpan without justification. DynSpan needs that fork because it derives project_id from @PROJECT_ID@, which is meaningless on the portal. This record takes project_id from webready.GenomicSeqAttributes_p instead, so one query and one PK are correct everywhere. Unaffected: Task 8, which adds UniDB to the pre-existing eQTL segment search's attributesList scope. That concerns an existing search, not this record. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code review of Task 1 tightened the implementation in three ways that the design documents did not yet reflect. The strain capture group now excludes underscore. The FASTA key is <strain>_<refSeq> and reference sequence IDs legitimately contain underscores (Pf3D7_01_v3), so without that exclusion strain A_B with sequence C and strain A with sequence B_C both yield the key A_B_C. No current strain name contains an underscore, so this is inert today and turns a future ambiguous key into a parse failure instead of a silently unfindable sequence. Validation moved into the private constructor and gained a refStart < 1 check. A zero start reaches BedLine.locationToZeroBased() and emits chromStart = -1, a malformed BED line with nothing pointing back at the ID that caused it. The original plan only checked start > end. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The search is handed a reference sequence ID, so also being handed an organism was redundant input that only created a consistency question the query then had to answer. Dropping it removes three problems at once. No partition-key concern: the ID query now resolves sequences through unpartitioned dots.ExternalNaSequence rather than webready.GenomicSeqAttributes_p, which is the same table path the attribute query already used, so the two are consistent instead of divergent. This design now touches no webready.*_p table. No @PROJECT_ID@: project_id for the primary key comes from apidb.organism.project_name via taxon_id, so one query stays correct on every project. No vocabulary-shape trap. With an organism param, org_abbrev IN ($$organismSinglePick$$) is wrong, and silently so. That param defaults to organismVQ.withGenes, a tree vocabulary projecting string_agg(abbrev, ', '), so a leaf term yields one abbrev but any grouping node yields 'afumAf293, afumA1163' — a single quoted literal matching no row, with no error logged. The usual dodge is overriding queryRef to a flat abbrev vocabulary, but every such vocabulary carries project scoping, which this record may not have. The original requirement to validate the reference sequence against the reference organism is now satisfied by construction: the EXISTS gate proves the strain has indel data on that exact sequence, so strain and sequence are consistent without trusting a supplied organism. Strain vocabulary becomes global: 6,119 strains in 2.9s, cached once instead of once per organism, so cheaper in aggregate than the 2.66s-per-organism version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A reference sequence ID already determines its organism, so taking an organism as well was redundant input. Resolving the sequence through unpartitioned dots.ExternalNaSequence removes the partition-key constraint, and taking project_id from apidb.organism.project_name avoids @PROJECT_ID@. The strain vocabulary becomes global; relevance is enforced by the ID query's EXISTS gate.
Deriving from study.protocolappnode with an EXISTS probe into indel_ix1 replaces a scan of all 43.6M apidb.indel rows: 2802ms to 57ms for an identical 6119-strain result set. Renames the query, whose StrainsByOrganism name no longer described it. Records why CONCAT is applied to the parameter rather than the column, and that the apidb.organism join is also a gate.
…ation One protocol_app_node_id spans every sequence of a strain, so grouping the offsets subquery on (strain, ref_start, ref_end) collapsed two segments on different sequences into one group and summed their shifts into both. Grouping and joining on source_id makes one-row-per-PK true by construction. Also clamps strain_length at zero for a segment falling inside a deletion, where the boundary rule legitimately inverts start and end, and expands WDK_ID_SQL once via a CTE.
Adds sections 5.3.1 and 5.3.2 covering two findings from review of the attribute query. The offsets subquery must group and join on source_id, not on the coordinate triple. One protocol_app_node_id spans every sequence of a strain, so keying on (strain, ref_start, ref_end) conflated two segments on different sequences and summed their shifts into both, silently. Keying on the whole primary key makes one-row-per-PK true by construction. A segment lying inside a deletion legitimately inverts, because the boundary rule shifts the end but not the start. strain_start and strain_end report the true inverted values so the reporter can reject them; only strain_length is clamped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> EOF
…e claim Follow-up review findings on StrainSegmentAttributes.Coords. Correctness: - Sum the offsets over a DISTINCT id set (new `segs` CTE) instead of the raw ##WDK_ID_SQL## output. A repeated source_id joined every indel row once per duplicate and GROUP BY source_id collapsed them into one group whose SUM was multiplied by the duplication factor -- silently. PK 366.1:Pf3D7_10_v3:331757-331757:f supplied twice gave offsets -198/-306 instead of -99/-153. Not reachable through today's ID query, but keying on the PK is meant to make one-row-per-PK true by construction, not by an argument about the input. Documented invariants (the code was right, the stated reasons were wrong): - strain_seq_id: the comment claimed strain names contain no underscore, so the first '_' ends the strain name. Re-measured: of the 6,119 strain names with indel data, 1,494 (24%) contain '_' (1_01_01, Af293_resequence2, China_LZCH-36, USGS_28834_1_NV) and 0 contain ':'. The concatenation is still correct -- it must match the dnaseq FASTA defline -- but the key is OPAQUE and must never be split back apart; consumers parse the strain out of the ':'-delimited PK. 0 actual key collisions today. - Collision detector: sharpened. All 126 duplicated strain names already have two protocol app nodes each carrying indel rows; what keeps the detector dormant is that no (name, na_sequence_id) pair is served by two nodes (0 today). That is the invariant the na_sequence_id join relies on. - StrainSegmentFeatureProvider rejecting the deletion inversion is retensed as a Task 6 requirement -- the class does not exist yet, so nothing rejects it today. Simplification: - offset_end drops its tautological CASE (the '<= ref_end' bound is already in the WHERE; verified 0 rows disagreeing) so the boundary asymmetry lives in exactly one place. offset_start keeps '<' -- that asymmetry is real. - strain_length is computed in an outer SELECT from the named strain_start / strain_end columns rather than repeating their offset arithmetic, so the length cannot drift from its own endpoints. - Hoist na_sequence_id/taxon_id into the `ids` CTE and drop the second dots.ExternalNaSequence join, halving the per-PK index scans and leaving one enforcement point for "unknown reference sequence yields no record". Verified against genomicsdb_rebuild01 (SELECT/EXPLAIN only): NRZ-2016-071 Chr1/Chr2 still 100012/100014; the 1-bp deletion PK still 331658/331604 length 0; three PKs differing only in strand still 3 rows; the same PK twice now yields 2 rows with single-copy offsets; EXPLAIN ANALYZE still uses indel_ix1 with `ids` materialized once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two documentation defects found by review, both of which would have misled the next reader: - §5.3 listed only the five derived columns and §6 said "exactly those §5.3 emits", which read literally forbade the ref_* attributes that §7's defline consumes. The query has always emitted eleven columns; the spec understated it. Now names all eleven and says why the primary-key fields are passed through rather than re-parsed. - "126 strain names resolve to two protocol app nodes" is imprecise: 122 resolve to two and 4 to three, which is where §2's count of 256 nodes comes from. Corrected in the spec (twice) and in the collision-detector comment. The invariant that keeps the detector dormant is unchanged: no (name, na_sequence_id) pair is served by more than one node.
':' is the only delimiter in the strain segment primary key, so a
reference sequence whose source_id contains one produces an ID that the
attribute query mis-parses and then dies on -- 'ncRNA'::integer aborts
the ENTIRE attribute query, taking out every row on the page rather
than just the offending record.
10,704 dots.ExternalNaSequence source_ids do contain ':' (transcript
features such as bld68_Tb927.1.05:mRNA). None of the 20,823
indel-bearing sequences do, so this is unreachable today -- which is
the reason to gate it rather than a reason not to: the grammar should
not depend on which features happen to get indel-called. Verified the
gate refuses 0 indel-bearing sequences and that all 8 Af293
chromosomes still pass.
Also corrects three comments the review found under-evidenced or
unverifiable:
- the colon-unambiguity claim covered "the strain or the sequence" but
measured only strain names; now measures both, and points at the gate
- the FASTA defline was quoted as '{sample}_{chrom_name}', which reads
as a verbatim f-string; now matches the spec's notation and cites
makeConsensusFastaFromVcfAndBed.py:233
- documents the sres.TaxonName inner join as the second silent
row-drop point, and the cross-query inconsistency that the ID query
resolves organism via apidb.organism while this one uses TaxonName
The record class is deliberately bare: primary key, idAttribute, and the nine non-PK columnAttributes from StrainSegmentAttributes.Coords. No tables, no summary view, no display attributes, so a record-page request fails rather than rendering an empty page. No includeProjects/excludeProjects anywhere -- unlike DynSpanRecordClass, project_id comes from the data, so no per-project fork is needed. The bed reporter element is NOT included yet: ReporterRef.resolveReferences does a Class.forName on the implementation at model-load time and throws, so the element cannot land before BedStrainSegmentReporter does. Task 6 adds both. No individuals.txt row, which is what keeps the search out of the category tree while leaving it addressable by name through the service API. wb model therefore suffices; wb ontology is not required. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Task 5's spec review found two plan defects with real downstream cost, plus staleness that had already caused one live bug. Task 6 could not have succeeded as written: - Its Files list named only the two ApiCommonWebsite Java files, with no step registering the <reporter> element on the record class. Task 5 had to omit that element -- WDK's ReporterRef.resolveReferences calls Class.forName at model-load time, so registering a class that does not exist yet hard-fails the build -- and the deferral note lived only in the XML and a commit message, neither on a Task 6 implementer's path. Without the registration the BED download is simply not offered, with no error; Task 7 would have failed on an unknown format. Task 6 now spans both repos and has explicit register + rebuild steps. - Task 6 now also states that rejecting the inverted interval is a required deliverable, not a nicety. The attribute query deliberately leaves strain_start/strain_end unclamped so a segment inside a deletion inverts; nothing upstream rejects it, so if the provider does not, an inverted interval reaches a FASTA lookup silently. Removed the stale organismSinglePick expectations from Task 5 Step 5 and Task 7 Step 3. There is no organism param; WDK rejects unknown parameters, so Task 7 would have failed on its first call. Corrected the false measurement at the root of the Task 1 regex bug. Task 2's preamble asserted that no strain name contains an underscore; 1,494 of 6,119 (24%) do. That claim propagated into the PK grammar as [^:_]+ and silently rejected a quarter of legitimate primary keys. The plan and spec now record the real numbers, the fix, and why narrowing the grammar could never have worked: reference sequence IDs contain underscores too, so <strain>_<refSeq> is not reversible at all. Also: Task 5 Step 5 claimed -showQuery would print the attribute query with ##WDK_ID_SQL## expanded. It does not, and that is correct -- run standalone through QueryTester there is no answer to supply the ID SQL. The step now says what it actually proves. And §3's project_id provenance named webready.GenomicSeqAttributes_p; the implementation uses apidb.organism.project_name.
Turn off the basket, drop a costly sort, and stop the comments claiming
guarantees WDK does not make.
- useBasket="false" on StrainSegmentRecordClass. The default is true
(RecordClass.java:303), so WdkModel.addBasketReferences injected
StrainSegmentRecordClasses_StrainSegmentRecordClass{Realtime,Snapshot}Basket
questions and user_baskets id queries, and RecordClassFormatter:75
advertised basket affordances for a class with no record page and no
saved strategies. Matches fileRecord.xml, userfileRecords.xml,
ajaxRecords.xml; verified gone from wdkXml -model FungiDB.
- Drop sorting="strain_seq_id asc". AnswerValue.prepareSortingSqls splices
a QueryColumnAttributeField's attribute query into the ordering SQL, so
it re-ran the apidb.indel prefix-sum aggregation just to order rows.
Undeclared, WDK sorts on the idAttribute against the id query alone;
strain_seq_id is a pure function of the PK, so nothing is lost.
- Correct a false precedent: DynSpansByLocation is only an sqlQuery with
no question and no references anywhere, so it says nothing about
service-API reachability. Cite DynSpansBySegIds alone, and record in
spec 5.1 that the param shape is modeled on a query no question has
ever exercised.
- Rewrite the record-class comment as intent, not a guarantee: WDK always
injects a _default summary view, a _default "Overview" record view, and
DefaultJsonReporter, and the nine columnAttributes are display
attributes (they must stay so; the BED reporter reads them).
- Spec: add the model-side <reporter name="bed" scopes="results"> element
as a third Task 6 deliverable and track it in the risk table, since a
missing reporter means no BED download and no error anywhere.
- attributesList summary now mirrors what the BED download consumes; add
a <description> alongside <summary>; note that the querySet
StrainSegmentId is unrelated to the Java class of the same name.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rewrite that closed the previous round introduced a new false claim of exactly the kind that round existed to remove, plus two overclaims: - The naming-caution comment, whose only job is to stop a reader conflating the querySet StrainSegmentId with the Java class of the same name, gave the class's package as org.apidb.apicommon.model.StrainSegmentId. Nothing exists there; it is org.apidb.apicommon.model.report.bed.util.StrainSegmentId. A disambiguating comment that resolves to nothing is worse than none. - "no reference anywhere in ApiCommonModel..." for DynSpansByLocation was absolute and false: the Model/vp2TuningTablesEffort inventories name it twice. The substantive point survives and is stronger for being precise, because tableUsageMap.json records an EMPTY usage list for it -- an independent inventory agreeing nothing uses it. - The attributesList comment claimed the summary "mirrors exactly what the BED download consumes". It does not: per spec section 7 the provider needs four attributes and reads strain and ref_seq out of the primary key instead, so the claim was false by two columns against the section it cited. It also cited getRequiredAttributeNames() in the present indicative for a class Task 6 has not written yet. Also: two line cites had drifted (addBasketReferences is at 646-652, prepareSortingSqls spans 748-824); the sorting fallback now traces from Question.getSortingAttributeMap, where the removed attribute actually lived, rather than starting at the record class; and spec 5.1.1 attributed source_id_uniq to dots.ExternalNaSequence, which is a view and can hold no index. The index is on the base table nasequenceimp, making the uniqueness guarantee broader than the spec claimed. Verified: wb model builds and the webapp reloads.
scopes="results" only, not "results,record" as dynSpanRecord.xml and genomicRecords.xml use: this class has no record page to download from. name="bed" is the literal reportName a download request passes, so it must match exactly, and nothing references the reporter by name at build time, which is why omitting the element yields no BED download and no error anywhere. Verified with wb model: ReporterRef.resolveReferences Class.forName's the implementation at model-load time, so a successful load is the proof that BedStrainSegmentReporter is compiled into the deployed webapp. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Task 6's first wb model failed with 'Implementation class for reporter bed ... cannot be found'. The XML registration was correct; the class had never been deployed. Verified in gus_home/bin/wb: the model target runs bld EbrcModelCommon/Model and bld ApiCommonModel/Model only, site covers Website/Site, ontology adds Presenters/Model, and no wb target builds ApiCommonWebsite/Model at all. mvn test compiles to target/classes, which is not on wdkXml's classpath. Worth documenting because the error names the wrong file: it reads as a bad reporter element and invites reverting a correct registration. The fix is bld ApiCommonWebsite/Model first.
Spec section 7 showed the two coordinate ranges as 'ref 100 to 200 | strain 143 to 241'. Nothing emits that. DeflineBuilder.appendPosition renders '<chrom>, forward strand, 100 to 200', so matching the example would have meant adding a builder method or hand-rolling strings, and the invented form silently dropped the strand. The name column is declared Free and the normative requirement is only that it carry provenance through DeflineBuilder honouring RequestedDeflineFields, which the implementation does. Corrected so nobody 'fixes' working code to match a retired example, and so Task 7 compares against the real string.
Task 6's re-review found the inverse of the usual defect: comments that were accurate when written and that landing Task 6 made false. Each was scoped "as of this commit", which is defensible authorship but reads as wrong to anyone looking at the current tree. The consequential one is in strainSegmentAttributeQueries.xml, sitting directly beside the deliberately unclamped strain_start/strain_end and telling the next reader that nothing downstream rejects an inverted segment. Something does, since a3c062e03. Leaving those columns unclamped is safe ONLY because that guard exists, so the comment now says that explicitly and names all three conditions the guard checks. Spec 5.3.2 likewise flipped from "Not yet implemented" to implemented, and now enumerates the full contract: the deletion inversion, strain_start < 1, and the strain_seq_id-versus-primary-key agreement check added during hardening. It also states the converse, which is the part worth keeping: if that guard is ever removed, these columns must be clamped instead. Also hardened the plan's test invocations. This module inherits surefire 2.12.4, where -Dtest=A+B matches ZERO tests; combined with the plan's -DfailIfNoTests=false that reported BUILD SUCCESS having run nothing. Switched to -DfailIfNoTests=true, corrected the stale "Tests run: 12" expectation to the 20 + 10 = 30 actually observed from a comma-separated run, and recorded the trap where someone will hit it. Verified: wb model builds and the webapp reloads.
Found while starting Task 7. Every SQL fact in this spec and plan was measured against genomicsdb_rebuild01 (localhost:5439), but the site resolves its appDb through LDAP genomicsdb_devn, which is genomicsdb_070n on ares13. On that database apidb.indel does not exist and there are 0 _Indel protocol app nodes. Consequence: the ID query's EXISTS over apidb.indel does not return zero rows on the deployed site, it raises "relation does not exist". Task 7 Steps 3-6 (run the search, download BED, prove the coordinates shifted, check the defline) cannot run there. Steps 1-2 read model metadata and are unaffected. What still stands: the queries were verified against a database that really holds this data, so the SQL correctness work is not in doubt. What was never established is that any deployed site can execute them. Also worth recording: the earlier switch from giardiadb to fungidb was justified by querying rebuild01, so it picked the right data but said nothing about either instance's actual appDb. Deliberately not worked around -- pointing an instance at a different appDb is John's call, and model-config.xml is conifer-generated rather than hand-edited.
… failed The instance was repointed at unidb_shu_a, which does have indel data (1,855,449 rows, 452 strains, 120 sequences, 13 organisms) -- a third database, so the spec section 2 figures, measured on rebuild01, do not describe it. Recorded the real numbers and two verified test cases for Steps 3-6, including the required offset_start=0 / offset_end!=0 case. Also recorded why the first gated rebuild still failed: the flag was present but the gate implementation is not on master. It lives on the dnaseq-merge-experiments branch, so presenterInjectTemplates ignored the property, injected a presenter whose dataset is absent from apidb.datasource, and the resulting EDA meta query referenced a table that does not exist.
IsolatesHTS no longer describes how DNASeq datasets are processed on this branch,
and both of its methods fail on the new shape:
- injectTemplates() derived per-sample gbrowse/jbrowse databases from getSampleList(),
which keys samples on organismAbbrev + datasetClassCategory + experimentName. The
new dnaseqExperiment class carries an empty category while its samples span two
("Genetic variation" for SNPs, "Structural variation" for CNVs), so the key cannot
match and getSampleList() throws "No sample names found for dataset ...".
- addModelReferences() registered SnpQuestions / SnpRecordClasses references. That XML
is not in the compiled model and is to be superseded by the variation record.
Both bodies are commented out rather than deleted, and the class stays in place as a
no-op so the presenters naming it keep resolving. getPropertiesDeclaration() is left
alone: presenters still supply hasCNVData, and DatasetInjector.addPropValues validates
supplied props against the declaration.
Note the deeper issue for whoever writes the replacement: keying the experiment ->
sample lookup on a single datasetClassCategory cannot work for an experiment that
aggregates SNP and CNV samples, even once dnaseqExperiment declares a category.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 9968576)
Commit 643ddb4 commented out the CommunityCount intermediate table and the communityCount column of OrganismAttributes in apiTuningManager.xml (it read from a hardcoded usercomments_userdb_betan.mappedComment), but left every consumer in place, so model load failed with: Database error while attempting to parse sqlQuery OrganismAttributes.organismAttrs: ERROR: column oa.communitycount does not exist Consumers brought into line with the producer, following the snpcount/popsetcount precedent already in these files: - organismAttributeQueries.xml: the communitycount column declaration and its select expression are commented out. - organismRecords.xml: the communitycount columnAttribute is commented out. - organismQuestions.xml: communitycount removed from five summary attribute lists, which is mandatory since WDK resolves every attribute a summary names. community_flag is kept, as a literal 'none' rather than derived from the missing column. That is exactly what "CASE oa.communitycount WHEN 0 THEN 'none'" produced for a zero count, so the organism page renders as it would with no comments, rather than losing an attribute that organismRecords.xml:305 and its red-dot <img> still use. Deriving it from NULL would have been worse than removing it: CASE NULL WHEN 0 falls through to ELSE, so every organism would have advertised comments it does not have. The individuals.txt ontology entry is left alone, matching how snpcount and popsetcount were handled: the category tree tolerates entries for absent attributes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 60a3e36)
These two questions were written by hand in geneQuestions.xml while the attribute
their summaries name is generated by the presenter:
summary="...,exprGraphAttrpfal3D7_microarrayExpression_Derisi_TimeSeries_RSRC_expr_graph"
Expression.java:136 builds that name and injects the textAttribute that defines it. So
a hand-written question depended on generated output, coupled only by a name string
with nothing enforcing that both exist. On an instance where this dataset is not
loaded, the presenter is skipped and the question survives, referencing an attribute
nobody generated:
Summary attribute field [exprGraphAttrpfal3D7_..._expr_graph] defined in question
[GeneQuestions.GenesByMicroarraypfal3D7_..._RSRC] is invalid
The questions are curated rather than generic -- they use the PFTimeSeries vocabulary
queries, an extra samples_fc_comp_generic param, and fold_change_chosen_display in
place of fold_change_chosen -- which is why the presenter sets hasMultipleSamples,
hasPercentileData and hasPageData to false and why they were hardcoded in the first
place. So they move to their own microarrayDeRisiTimeSeries.dst rather than being
folded into the generic expression templates, and MicroarrayDeRisiTimeSeries injects
them. Question and attribute now appear and disappear together.
Only three values are parameterised -- ${datasetName} for the question and graph
attribute names, ${includeProjects} and ${includeProjectsExcludeEuPathDB} for scoping,
both already set by Expression.java to precisely the literals that were hardcoded.
Display strings stay literal because they describe this experiment. addModelReferences
now derives the question names from getDatasetName() rather than spelling out the
dataset, so the references cannot drift from what is injected.
Not yet verified: that the generated question is equivalent to the deleted static XML.
Nothing is generated on an instance where this dataset is absent, so this branch can
only show that its absence is handled. Before this reaches a full-data site, run a
build with the dataset declared and diff the generated question against the static
version in this commit's parent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 28b06e4)
CompoundQuestions.CompoundsByFoldChange named one dataset's graph column in its summary:
summary="...,metaboliteProfiles_LlinasMetabolites_RSRC_metabolite_graph"
That attribute is injected per metabolomics dataset by metabolomics.dst, so naming a
particular dataset's copy in a generic search has two consequences: model load fails
wherever that dataset is not loaded ("Summary attribute field [...] is invalid"), and
where it is loaded the search shows that one dataset's graph regardless of what the
user searched for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit e785390)
End-to-end verification failed with PSQLException "syntax error at or near WITH". Cause: two comments in the attribute query mentioned the ##WDK_ID_SQL## macro by name. WDK substitutes it by blind textual replacement, so it expanded inside the comments too, and because the injected SQL is multi-line it escaped the '--' and corrupted the statement. One of those comments was added during review to document that row counts match the injected id SQL -- a comment written to explain correctness is what broke the query. Notably wdkQuery -showQuery cannot catch this: standalone it never expands the macro at all, which this plan had already recorded as correct behaviour. Only a real answer run reaches it. Added a warning at the top of the query telling the next person never to write the macro name in a comment here. Also corrected the spec's claim that the pipeline emits one merged multi-strain FASTA. The delivered artifacts are one gzipped file per strain; deflines verified byte-identical to the chrom this record emits.
Established by reconstructing strain sequence from reference with samtools and comparing against the real consensus FASTAs: the only deletion offsets that reproduce the observed strain sequence start at location+1, never at location. Verified on two independent events (A17-10A-1 offset 1207 -> valid 1208-1212; E-1-75s-2 offset 2492 -> valid 2493-2495). This confirms rather than contradicts the < / <= asymmetry in section 5.3: an event whose location equals refStart deletes bases starting at refStart+1, so it is genuinely inside the segment and must shift the end but not the start. Also records two caveats the exercise exposed. Event position is ambiguous within repeat context (a 5-wide window of offsets reproduces the same consensus), which is the honest answer to the deferred refStart-boundary question -- an event straddling a boundary has no exact strain coordinate because the reference base does not exist in the strain. And only the net shift is meaningful: on an insertion region a clustalo alignment resolved recorded events -1,-6,+31,-1 as -1,+25,-1, identical in sum but not one-to-one.
The EXISTS gate in StrainSegmentId.StrainSegmentsByRefSegment matched on na_sequence_id, so it did two jobs: prove strain and sequence are mutually consistent (required, and what lets this search drop the organism param) and prove indel data exists on that exact contig (not required, and wrong). Zero indels on a contig is an ordinary valid state, not missing data: the strain matches the reference there, so strain coordinates equal reference coordinates and the contig is present in the strain's consensus FASTA. Those requests returned an empty result: 689 of 6,068 valid strain/sequence pairs (11%). Surfaced by A17-10A-1 + mito_A_fumigatus_Af293, whose defline is in A17-10A-1_consensus.fa.gz. Match on taxon_id instead. The gate now proves the strain was sequenced against this organism. The substitution is exact, not approximate: (strain name, taxon) resolves to exactly one protocol app node in 452/452 pairs and no protocol_app_node_id spans more than one organism. The attribute query needs no change; its LEFT JOIN + COALESCE already yields the identity mapping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Plain-text summary of Task 7 verification against the running site: service checks, BED coordinates for five strains on Chr1 and mito, the clustalo alignments confirming the recorded indels, the unshifted control, and the VCF-anchor-base finding.
The report's audience reads the alignment against the reference and never sees apidb.indel, so the 3-4 bp difference between a recorded indel location and the aligned gap position is internal noise that reads as an unexplained discrepancy. Removed it and the anchor-base derivation, keeping the parts that describe the output: gap sizes match the recorded shifts, and a segment boundary falling inside a deletion has no exact strain coordinate. The full derivation stays in the design spec, where it belongs.
…ones Documentation-only pass over the organism-scoping change; no SQL semantics touched. Two code reviews agreed the SQL is right and every finding was a number or a comment that described a different database. - Separate unidb_shu_a (live appDb) from genomicsdb_rebuild01 everywhere the two were conflated: strain-name ambiguity (0 of 452 live vs 126 on rebuild01), colon-bearing source_ids (0 of 160,581 vs 10,704), indel-bearing sequences (120 vs 20,823), row counts (1,855,449 / 1,356 vs 43.6M / 61,512). - Enumerate all FIVE ID-query gates, in the spec's numbering, in both the spec and the XML header, so a cited "gate 3" means the same gate in both. - Measure and record the REJECT path, which the old "0.35 ms, no sequential scan" claim omitted: ~101 ms vs ~53 ms for the largest strain, scaling with the strain's indel row count. Record the rejected LIMIT 1 rewrite and why (it trades slow for silently wrong). - Replace the Af293-only safety evidence with a three-organism check including TREU927, where 144 newly-admitted pairs sit on 36 never-indeled contigs and all 131 deflines match the DB set exactly. - Reframe the residual assumption as set equality that can drift from the DB side too, and add it to the section 10 risk register. - Fix the stale param comment describing the removed exact-sequence gate, the "8 Af293 chromosomes" (it is 9), and a below/above cross-reference. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cut roughly a third, and lead with what the thing actually is: a search taking a strain name plus reference coordinates as input and returning the segment in that strain's coordinates. Previous version described the internals before saying what was being run.
Same content, rendered: tables for the identity figures and fenced blocks for the BED lines and alignment excerpts, so it reads properly in the PR and on GitHub.
StrainSegmentId.StrainSegmentsByRefSegment took a single strain, so a five-strain BED meant five searches. The strain param becomes multiPick and the ID query crosses the sequence with the unnested strain list, minting one primary key per strain. Downstream was already multi-record capable (StrainSegmentAttributes.Coords keys on source_id; BedReporter streams one line per record) and is untouched. quote="true" is what makes this work: on PostgreSQL, EnumParamHandler + DBPlatform.prepareExpressionList render the selection as 'a','b','c', which is exactly an ARRAY[...] element list. Verified against the running service. The organism gate keeps pan.name on the bare side of an equality (CONCAT(st.strain, '_Indel')) so pan_named_ix stays an Index Only Scan, one index search per strain, rather than a regexp_replace that would force a scan. Gates now run per (sequence, strain) pair, so an irrelevant strain drops out of a mixed selection instead of failing the search. maxSelectedCount="500" is a correctness rail: above the whole 452-strain vocabulary on unidb_shu_a so it refuses nothing today, but below Oracle's EXPRESSION_LIMIT of 999, past which prepareExpressionList emits a form ARRAY[...] cannot consume. Verified end to end against jbrestel.fungidb.org (bed reporter): 5 strains on Chr1_A_fumigatus_Af293 396000-399000 reproduce the known single-strain coordinates exactly; 1 strain unchanged; 5 strains on mito 500-2000 all identity-mapped; a PlasmoDB sequence yields 0 rows; an out-of-organism strain mixed with two valid ones yields 2 rows; all 452 selected yields 232 distinct rows in ~2.3 s. Cost on unidb_shu_a: 1 strain 238.24 / ~0.35 ms, 5 strains 939.65 / ~0.61 ms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Passing a real JSON array returns 400 '"strain" is not a string (class org.json.JSONArray)'. Worth stating in the report because it is the first thing a caller gets wrong, and the multi-strain search is useless until they get it right.
The mito table reported E-1-75s-2 at 99.933% but the excerpt (columns 401-460) contained no difference, so the report asserted a mismatch the reader could not see. 99.933% of 1501 is exactly one SNP; it is at column 700, T -> A. Swapped the excerpt for a window containing it and marked the position. Also stated once that identity is measured over the whole alignment while the blocks are windows chosen to show a feature -- which is what made the discrepancy confusing rather than obvious.
jbrestel
marked this pull request as ready for review
August 1, 2026 02:15
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an internal, non-user-facing WDK record class and search: given one or more strains plus a reference-coordinate location (sequence, start, end, strand), it returns each segment in that strain's consensus-sequence coordinates as a BED feature — one line per strain from a single search. Coordinates are derived by prefix-summing
apidb.indel.shift.Companion PR: VEuPathDB/ApiCommonWebsite#307 (BED reporter + feature provider). Neither works without the other — the
<reporter>element here names a class from that PR, and WDK resolves it at model-load time.Verified end to end on the FungiDB dev instance
Full write-up:
docs/superpowers/2026-07-31-strain-segment-e2e-report.md.chrombyte-matches the deflines in the strain consensus FASTAs (<strain>_<refSeq>) — the constraint the whole feature exists to satisfy.clustaloalignment of the real consensus sequences at the returned coordinates reproduces the recorded indels exactly: gap sizes match the recorded shifts, and strains with no recorded event show no gap.Design decisions worth a look
includeProjects/excludeProjectsanywhere, including the PK columns.project_idcomes from the data (apidb.organism.project_name), never@PROJECT_ID@— so no UniDB fork of the kind DynSpan carries.individuals.txtentry. That omission is what keeps the search out of the category tree while leaving it reachable by name via the service API.useBasket="false"— otherwise WDK injects basket questions for a record class that supports no saved strategies.strain_start/strain_endare deliberately unclamped so a segment inside a deletion inverts; onlystrain_lengthis clamped. The feature provider in #307 rejects the inversion, so that guard is load-bearing — if it goes, these must be clamped instead.<strain>_<refSeq>is opaque and must never be split — strain names contain underscores. Consumers parse the primary key, where:delimits unambiguously.Note on four cherry-picked commits
This branch carries four commits from
dnaseq-merge-experiments(the DeRisi.dstinjection, the compound-search attribute, the dormant DNASeq injector, and thecommunitycountconsumer). They are required for the dataset gate to work on a partial-appDb dev instance, and will also arrive via that branch's own merge.Simulated both merge orders against current
master: no conflicts either way, and both orders produce an identical tree. Worth re-checking at merge time if those commits change on the other branch.