Repository navigation
Move the CLI's input handling into the library - #37
Merged
Merged
Conversation
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…izes Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This was referenced Oct 21, 2026
jerome-white
added a commit
that referenced
this pull request
Oct 22, 2026
* Make Tables a dependency of the library; DataFrames a test dependency #41 makes the library accept Tables.jl tables, which needs Tables.jl at run time. Tables was a dependency until #29, which moved it to the test extras because nothing used it; it comes back to [deps]. The [compat] entry (Tables = "1") was kept in #29, so it needs no change. Tables.jl is small and pure Julia, and it is the interface DataFrames, CSV and others implement, so the library still depends on none of them. DataFrames becomes a test-only dependency. #41 promises that a DataFrame works, and the literal claim is worth a test; the library never loads it. Tables stays available to the tests as a dependency of the package. Refs #41 Part of #38 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Accept Tables.jl tables in randomized_tukey_hsd Adds randomized_tukey_hsd([rng,] table; trials, block, treatment, value) in src/tables.jl, so a DataFrame (or any Tables.jl table) can be passed as it is and the treatment names come with the data instead of through a separate systems argument. The shape is told by which column keywords are given: - none: wide, each column is a treatment (named by the column name, as a string), each row a block; only `block` leaves out an id column; - all three of block, treatment and value: tidy, one row per block and treatment, in the order they first appear; - anything else is an ArgumentError that says what is expected. Column names may be Symbols or Strings. There are deliberately no IR default names (topic, system, score) in the library; the CLI keeps them. The matrix method is untouched and remains the core: the table methods only convert (with scores_matrix and wide_scores from #37) and call it. Julia picks the matrix method for a real matrix because it is more specific, although a matrix is itself a table. A matrix whose elements are not real (say Union{Missing,Float64}) would otherwise be read as a table with columns named Column1, Column2, ...; it is rejected with a message that suggests NaN instead. A missing score, or a pair with no row, becomes NaN and is rejected by the existing NaN check, the same as for the matrix method. The two helpers are underscored and unexported. The positional tuples that scores_matrix and friends return are left as they are; their shape is a question carried on #41. Refs #41 Part of #38 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Test the table methods against the matrix method test/table_input.jl builds tables holding the same 4 topics x 3 systems scores as the matrix tests and requires the same results for the same generator state, which is a stronger check than "looks plausible": - wide, as a NamedTuple of columns, with and without an id column left out by `block` (given as a Symbol and as a String); - tidy, equal to the matrix call when rows come grouped, and, with rows in a shuffled order, the same pairs and differences up to the last bit (the topics are summed in another order). The p-values are not compared in that case: another order of appearance puts treatments in other columns, so the same generator state shuffles differently. The test says so; - other kinds of table: a vector of named tuples, and a DataFrame both wide and tidy (DataFrames is a test-only dependency, see the previous commit); - the default generator, seeded with Random.seed!; - a missing score and an absent pair end in the NaN rejection; - a real matrix still takes the matrix method, and a matrix that is not real (Union{Missing,Float64}) is rejected with the NaN hint, not read as a table with columns Column1, Column2; - errors: not a table, unknown column (named, with the available ones), tidy roles given partly in each way, the same column in two roles, trials = 0 through the table path, and `systems=` is a MethodError because the names come from the table. Refs #41 Part of #38 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Document passing a table in the README Adds a "Passing a table" section under "Using the library" with the wide and tidy calls, what names the treatments in each, how rows are ordered, and what happens to missing scores. It sits after the paragraph that describes the result, so that paragraph stays next to the call it is about, and the installation note now lists Tables among the library's dependencies (Random, Statistics and Tables). The examples use a DataFrame called df, as the section says any Tables.jl table works and the library does not depend on DataFrames. Refs #41 Part of #38 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Name the treatments of a wide table by the Symbols the table gives The wide path converted the column names to Strings before reporting them. The reason given on the PR was that this matched the matrix path and the CLI, and on review that turned out not to hold: - the CLI never goes through this method; it reads wide data with wide_scores and the names DataFrames gives it (already Strings); - the matrix path has no preferred type, since systems= takes whatever the caller passes and defaults to the column numbers; - CSV writes a Symbol as text, so no output needed the conversion. It came from adapting the library to a habit of the CLI side, where the library should follow Julia: Tables.jl names columns with Symbols, and the tidy path already returns the treatment values as they are. The wide path now does the same and reports the names as the table gave them. The wide tests compare with a matrix call whose systems are the Symbols, and check the pairs by Symbol; tidy results are unchanged. The docstring and the README say Symbol. The CLI is not affected. Refs #41 Part of #38 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
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
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.
Closes #36
What this does
Moves the logic of
bin/rthsd.jlthat needs no I/O dependency into the library, as two unexported plain-array functions in a newsrc/input.jl(reachable asRandomizedTukeyHSD.name). The library gains about 70 lines (with docstrings) and no dependencies; nothing on the hot path changes.scores_matrix(topics, systems, scores)unstack+select!+Matrix{Float64}(df)incomplete_topics(scores)incomplete(df)bin/rthsd.jlis now ArgParse, CSV in and out,--seedtoXoshiro, and logging, around calls to those.The arrangements count is removed
The plan on #36 also moved
log10_arrangements. On review it was dropped instead (the last commit): it was added to the Python version as a cosmetic hint at how large B would have to be to cover the whole space. It is only informative when the space is small enough to enumerate, which is the test fixtures; at realistic sizes it is 10^thousands (10^39 492 for 100 systems × 250 topics) and no B gets near it, and the accuracy of the p-values depends on B throughsqrt(p(1 - p) / B), not on the size of the space. It also took a bug fix (#28) and a dependency change to keep correct. The CLI's settings message now printsBandseed. If wanted for tiny cases later, it is a one-liner (factorial(nsystems)^ntopics).Behaviour decisions (as confirmed on #36)
unstackgave.missingbecomeNaNin the matrix, so they are treated exactly like aNaNscore (the earlier--missingbehaviour, now by construction).ArgumentError: more than one score for topic "t1" and system "A". Before,unstackrefused it with its own error (Duplicate entries in unstack ... Pass combine keyword argument), which pointed at an option the CLI does not have. This is the only user-visible change.DimensionMismatch.CLI output is unchanged (checked)
I ran the old and new CLI with
--B 500 --seed 1for--missing errorand--missing dropon seven inputs:tiny, one with aNaN, one with an absent row, one with an empty score cell, one with a duplicate row, one with numeric system names (1, 2, 3), and the 100 × 250 file. All stdout is identical. The only differences are the duplicate-row error message above and the logged settings, which no longer includelog10_arrangements.Tests (111 pass + 1 deliberately broken)
New
test/input.jl, with no subprocess and no extra CI job:scores_matrix: first-appearance order, absent pair,missing, numeric names and integer scores, both errors.incomplete_topics: flags, and that dropping them leaves a matrix the test accepts.Not done
🤖 Generated with Claude Code