Conversation
…w, we can compile it
After the async sampler removal these two keys were still written into the allParams list (and forced to defaults) but read nowhere -- neither R nor Cogaps.cpp consumes them. Remove them from the list and drop the now-pointless forcing lines. The CoGAPS/scCoGAPS/GWCoGAPS function arguments and the deprecation warning stay for backward compatibility (verified the warning still fires on nThreads!=1 / asynchronousUpdates=TRUE). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The sc/gw dimension tests passed an in-memory data.frame to distributed CoGAPS, which warns "running distributed cogaps without mtx/tsv/csv/gct data", and built a params object with nSets=2 that was never passed. Use the packaged GIST.mtx (same data) for the distributed runs and drop the dead params lines. Suite now runs with no warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bring the branch up to date with master (59 commits since 8a21f28) and resolve the 38 conflicts. Notable conflict resolutions: * src/cpp_tests/*: master independently landed the same Catch2 work (the 85-cpp-tests branch). The branch's versions are supersets -- 54 test cases vs 10, the six empty serialization stubs filled in, and the #if 0 blocks revived -- while master's still reference APIs that no longer exist (TsvParser.h, FileParser::writeToMtx), so they would not compile. Took the branch side; kept master's [AI-generated] comments where the surrounding code survived. * nPatterns is now a required argument (master, 24448b6). Kept that and threaded nPatterns through every CoGAPS() call in the branch's tests, and through new("CogapsParams") in test_parameters.R. * R/CoGAPS.R: master's signature (nPatterns, and the params default that uses it) combined with this branch's semantics -- the async sampler is gone, so nThreads/asynchronousUpdates stay only as ignored deprecated arguments. * Async-only files that master had merely re-commented (ProposalQueue, ConcurrentAtomicDomain, AsynchronousGibbsSampler) stay deleted. * configure.ac: the branch already contains master's improvements (AS_HELP_STRING, the cpp_tests_disable logic, the full test list) in a more complete form, so configure needs no regeneration. Also fixes a defect this merge exposed: setUncertainty() called pad(1.f), which fills *every* element rather than the SIMD tail, so a user-supplied uncertainty= matrix was overwritten with 1.0 and silently ignored. Two runs whose uncertainty differed by a factor of 100 returned an identical chi-square. Now uses padSIMD(1.f). The defect predates the branch and is present in master too; master's test_chisq.R case covers it and passes only with this fix. Written up as issue 20 in the branch notes. Verified: C++ suite 873438 assertions in 54 cases, R suite 128 passing / 0 failing, R CMD check level with master (2 WARNINGs, 5 NOTEs).
.vscode/c_cpp_properties.json is a per-machine IntelliSense config; the committed one points at Homebrew paths that do not exist on other setups. The file stays on disk, it is just no longer tracked. Also ignore built package tarballs.
R CMD check reported 'significantly better compression could be obtained' for GIST.RData, which this branch had resaved as gzip (345Kb). Resaving all three data files with xz brings GIST.RData back to 209Kb and removes the WARNING, putting the check level with master.
- Drop remove-async-plan-rus.md. It was a full translation of remove-async-plan-eng.md: same 31 sections, identical code blocks, no unique content, and the English one is marginally more detailed. - Fix the filename typo: after_asnc-removed-plan.md -> after_async-*. - Move static_cast_standalone_test.cpp out of src/cpp_tests/ into its own directory under the notes. It has its own main(), is not in the build, and is not a unit test, but sitting among them it looked like one and shipped inside the package tarball. Added a README explaining what it demonstrates, how to build it, and that the bug only reproduces on old Apple Clang on x86-64 -- on arm64 both values round-trip to UINT64_MAX rather than to 0, which is the benign outcome. - Update the references to the moved and deleted files.
The last entry was from 2015; this records the changes users would actually notice: the asynchronous sampler is gone (single-threaded only, nThreads/asynchronousUpdates ignored), the uncertainty model is fixed and consistent between the dense and sparse samplers, and a user-supplied uncertainty matrix is no longer discarded.
Guidance for Claude Code sessions in this repo: build and test commands (including the Catch2 runner and the configure toggles), the layer and dispatch overview, the sampler geometry, the uncertainty model and the pad()/padSIMD() trap, and the working agreements from .sasha-copilot-context/copilot-rules.md. Excluded from the package build along with .vscode.
Both wrappers still defaulted `params` to `new("CogapsParams")`, which
stopped being constructible once nPatterns lost its default of 7. Every
call failed with 'argument "nPatterns" is missing' -- including calls
that did pass nPatterns, because the first statement of each wrapper is
`params@distributed <- ...`, which forces the lazy default before `...`
is forwarded. So the wrappers only worked with an explicitly built params
object, and no amount of adding nPatterns= to the call would help.
Give them the same signature treatment CoGAPS() already got:
params = new("CogapsParams", nPatterns = nPatterns), nPatterns,
Rd files were edited by hand rather than regenerated: the installed
roxygen2 (8.0.0) is newer than the pinned RoxygenNote (7.3.3) and would
have rewritten all 77 of them. `checking for code/documentation
mismatches` passes.
Add test_deprecated_wrappers.R -- neither function was called anywhere in
the suite or in a runnable example, which is exactly why this went
unnoticed. It exercises both with and without an explicit params, and
checks each sets the distributed mode it is named after.
The defect comes from master (24448b6), not from this branch.
The same master commit (24448b6) raised nPatterns from 5 to 7 in this test but left the lp vectors at length 5. patternMarkers() then warned 'lp length must equal the number of columns of the Amatrix' and took the invalid-lp path, so the case meant to cover a well-formed lp never exercised it. expect_no_error() did not catch this -- a warning is not an error -- and the noise accounted for 2727 of the suite's 2730 warnings. Pad the two vectors that are supposed to be valid to length 7. The middle call keeps its length-4 vector: it asserts the length warning on purpose. Length drives the warning, a value above 1 drives the error, so the third call keeps its 2 and only grows. Suite warnings drop from 2730 to 2.
The date was set while resolving the master merge; move it to the day the branch is actually being finished.
R CMD check went from 5 NOTEs to 1 (all of them pre-existing on master):
* LICENSE now holds the DCF fields the BSD_3_clause template expects
(YEAR / COPYRIGHT HOLDER / ORGANIZATION) instead of a copy of the
licence text, which is what "License stub is invalid DCF" was about.
R supplies the canonical BSD-3 text itself.
* import stats::setNames, and declare the dplyr/ggplot2 NSE column names
(gene.set, padj, neg.log.padj) via globalVariables().
* escape {fgsea::gsea} / {fgsea::fora} / {patternMarkers} as \code{} so
Rd stops reporting lost braces.
* drop the @PARAM total documented on sampleWithExplictSets(), which
takes only allParams.
The one remaining NOTE is std::cout/printf in the compiled code; that
needs the C++ output moved to Rprintf throughout and is out of scope.
Also switch test_DistributedCogaps.R to the packaged GIST.mtx: passing an
in-memory matrix to distributed CoGAPS warns, and those were the last two
warnings in the suite. Same data, so the dimension assertions still hold.
NAMESPACE and the two Rd files are edited by hand rather than regenerated,
for the roxygen-version reason noted earlier.
Three long-standing defects in exported functions, all present at the branch point (8a21f28) and still in master. They surfaced only now because none of these functions was called from any test: 21 of the 37 exported functions had no coverage at all. * binaryA() called calcZ(object) without the mandatory whichMatrix, so every call died with 'argument "whichMatrix" is missing'. It thresholds the z-scores of the A matrix, so it now passes "featureLoadings". * fromCSV() read the four matrix slots with read.csv and handed the resulting data.frames to new("CogapsResult"). CogapsResult extends LinearEmbeddingMatrix, whose slots are matrices, so a toCSV/fromCSV round-trip returned an object with the wrong slot types. Wrapped in as.matrix(). * show() on CogapsParams referenced a bare `checkpointInFile` instead of object@checkpointInFile, so printing any params object with a checkpoint file set raised "object 'checkpointInFile' not found". This one was visible all along as the "no visible binding for global variable" NOTE. test_result_accessors.R covers all three as regressions and fills in the untested accessors around them: getFeatureLoadings/getVersion/ getOriginalParameters, calcZ (both matrices, the bad-argument error and the zero-stddev warning), reconstructGene checked against A %*% t(P), calcCoGAPSStat, and the toCSV/fromCSV round-trip. It runs 1000 iterations: shorter runs leave exact zeros in the standard deviation matrix, and calcZ then warns, which would make the file noisy.
These four helpers feed the sampler and GapsStatistics but had no direct coverage -- the last open item from after_async-removed-plan.md 3.2 (nonZeroMean, pmax and min/max were covered earlier in the branch). dot_diff runs a SIMD loop, so besides known values it is checked on a size of 13, which is not a multiple of SIMD_INC, and against dot(a, b - c). sparsity is checked for agreement between the dense and the sparse matrix overloads. The Catch suite goes from 54 to 58 cases. configure was edited by hand alongside configure.ac: autoconf is available but autoconf-archive is not, so regenerating would leave AX_COMPILER_VENDOR/AX_COMPILER_VERSION unexpanded. The GAPS_SOURCE_FILES lines are copied verbatim into configure, so inserting the same line is equivalent; the object lists in the two files are identical.
The committed configure still carried the package version from before the
master merge (3.27.2.132) in its header and PACKAGE_VERSION, and the
testMathHelpers.o line had been inserted into it by hand. Regenerated it
properly so the generated file matches configure.ac and DESCRIPTION.
Regenerating needs two commands, not one:
aclocal -I /opt/homebrew/share/aclocal
autoconf
configure.ac uses AX_COMPILER_VENDOR and AX_COMPILER_VERSION from
autoconf-archive, and it is aclocal that makes them available -- autoconf
on its own leaves them unexpanded, where they become literal shell
commands. The result still runs, but prints "AX_COMPILER_VENDOR: command
not found", leaves $ax_cv_cxx_compiler_vendor empty and thereby turns
--enable-warnings into a silent no-op. master's configure is in that
state; this branch's is not, and the note added to src/README.md and
CLAUDE.md is there to keep it that way.
Worth knowing: with a working --enable-warnings the build now genuinely
fails under -Werror, on -Wcast-function-type-mismatch raised inside
Rcpp's own routines.h under newer clang. No CoGAPS source file produces a
warning.
test_cpp.R already ran the Catch suite, so a C++ failure did fail
R CMD check, but it did so through a single expect_equal(runner(), 0L).
That had three problems: 58 test cases collapsed into one pass/fail, so a
failure said only "expected 0, got N"; Catch writes its report from C++
straight to stdout, where testthat cannot capture it, so it landed in the
run output as noise; and when no test case is compiled in the runner
returns 0, making the check pass vacuously -- which is the situation on
Windows, where Makevars.win lists no cpp_tests objects, and under
--disable-cpp-tests.
test-runner.cpp now takes an `output` argument, passed through to
Catch's ConfigData::outputFilename, and exposes catch_test_case_names()
over the Catch registry. Both existing entry points share one
implementation. Defaults are unchanged: output="" still writes to stdout,
so the interactive call behaves exactly as before.
CoGAPS:::run_catch_unit_tests() # console, as always
CoGAPS:::run_catch_unit_tests_by_tag("[vector]")
test_cpp.R asks for the xml reporter and a tempfile instead, parses it
with xml2 -- a suggested dependency that had been sitting unused since
"update testthat linking" -- and turns every <TestCase> into its own
expectation, named and located when it fails. If no case is registered it
skips with an explanation rather than passing.
Verified by temporarily breaking an assertion in testMathHelpers.cpp:
C++ test case failed: gaps::elementSq squares each element
(cpp_tests/testMathHelpers.cpp:23)
The R suite goes from 173 to 233 passing, and Catch's output no longer
appears in the run. Both ways of running the tests are written up in
src/cpp_tests/README.md, with a shorter note in CLAUDE.md.
This branch is about the uncertainty model but carries several fixes that have nothing to do with it, and a reviewer will reasonably ask why. The note answers that: for each one, the symptom, the cause, where it came from (24448b6 on master, or older than the 8a21f28 branch point) and the commit that fixed it here. It keeps the details that are expensive to rediscover -- that passing nPatterns did not rescue the wrappers because params@distributed forces the lazy default first; that expect_no_error() ignores warnings, so the lp test stayed green while testing nothing; that the show() defect had been sitting in plain sight as an R CMD check NOTE. It also adds a "known gaps left open" section that the maintainer-facing write-up did not have: nPatterns losing its default as a compatibility break, master's configure leaving the AX_ macros unexpanded, --enable-warnings failing on Rcpp's own headers under -Werror, Windows building no C++ tests, the remaining std::cout NOTE, the 12 exported functions still untested, and the open weighted-sampling defect.
CLAUDE.md gains a "Setting up a fresh machine" section. Nothing recorded what a working environment actually needs, so on a new machine it gets rediscovered by hitting failures. It lists the R/Bioconductor versions this was verified against, the install commands, and autoconf-archive, plus the three dependencies whose absence is not obvious up front: * testthat is in LinkingTo, not merely Suggests -- it supplies the Catch2 header, so without it src/ does not compile at all; * xml2 is what test_cpp.R parses the Catch report with, and being only suggested it degrades silently to a single pass/fail; * SeuratObject is needed for R CMD check, which will not run a complete check while a suggested package is missing. copilot-rules.md: commit permission applies to the request that granted it and does not extend to the next step. Added by the maintainer after I committed the configure regeneration without being asked -- the earlier turns had each ended in a commit and I carried that over.
The notes directory had grown to eleven files with no way to tell which is a spec, which a retrospective, and which an unresolved question. The README groups them and states the shared issue numbering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6 tasks
The notes were in a hidden, personally-named directory nested one level by issue number, which made them hard to find and, checked as a source directory, liable to a hidden-files NOTE. They now sit in a visible top-level dev-notes/, flat, still excluded from the build. .Rbuildignore, CLAUDE.md, the pointer in test_subset_data.R and the cross-references inside the notes follow the new paths. Verified that R CMD build leaves the directory out of the tarball. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AGENTS.md was written for Copilot before CLAUDE.md existed and was never updated: it called run_catch_unit_tests() exported, told the reader that autoconf alone regenerates configure, still listed OpenMP among the build options and pinned roxygen 7.3.2. Everything it said correctly was already in CLAUDE.md except two details about src/, which move there now: the header-only utils/ directory, and test-runner.cpp sitting in src/ rather than in cpp_tests/. copilot-rules.md is the one Russian-language file in the repository, so it becomes dev-notes/rus/.agent-rules-rus.md. The index and CLAUDE.md follow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The document was written after issues 17-19 and so described the custom uncertainty path as "padded with 1.f for SIMD safety" -- the very wording that hid the bug. It now names padSIMD(), records that pad(1.f) used to wipe the caller matrix, and points at the R-side chi-square test that catches a regression. Also corrected: the sparse model rejects uncertainty= rather than ignoring it, and the sparse line numbers in the code map had drifted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The leading dot only made it invisible to ls; rus/ already says what the file is, and .Rbuildignore excludes dev-notes/ whole either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The mean chi-square bug (issue #159), the batch-size parameter that does not reach BiocParallel, and making the uncertainty parameters uniform across storage modalities. Each entry carries pointers to where the digging should start, kept separate from the maintainer statement of the task itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The setup section listed the R packages only. It now names the Homebrew ones too, with what each is for: autoconf/autoconf-archive for configure, pandoc for the vignette that R CMD check builds, gh for issues and PRs, and the compiler coming from the Xcode Command Line Tools rather than from Homebrew. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A distributed run dies with "no gene names given" whenever the consensus collapses to a single pattern: stitchTogether re-orders with [reorder,] and no drop = FALSE, so a one-column matrix degrades to a vector and loses its row names. Reproduced on master 3.33.1 with nSets=11 on GIST, and on this branch with nPatterns=1 -- the branch differs only in that GIST no longer collapses, not in the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
stitchTogether re-ordered with [reorder,], which drops the dimensions of a one-column matrix and returns a vector; its rownames are NULL, so CogapsResult refused the result with "no gene names given". Reproduces on master with nSets=11 on GIST and here with nPatterns=1, under SerialParam as well -- it is not a parallelism problem. All four subscripts get drop=FALSE, and a distributed one-pattern run in both modes is now covered by a test that fails without the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found while diagnosing the "no gene names given" crash. A distributed run reports meanChiSq = 0 while the same data run plainly reports 101381; it isolates to a fixed matrix rather than to distribution, and stitchTogether then sums the zeros. Recorded under task 1 because it reproduces in one line and opens the same code path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… story here yet.
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
2 participants
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.
No description provided.