From c6d6ee10b6e724426e226a7e66fdf9298c2da49e Mon Sep 17 00:00:00 2001 From: Alex Maldonado Date: Sun, 27 Sep 2026 01:11:09 -0400 Subject: [PATCH] feat: improvement --- src/analysis/calibrate.rs | 146 ++++-- src/authoring/jsonschema.rs | 137 +++++- src/cli.rs | 39 ++ src/commands/analysis.rs | 51 ++- src/commands/project.rs | 107 ++++- src/lib.rs | 6 +- src/migrate.rs | 639 +++++++++++++++++++++++++- src/model.rs | 1 + src/model/bank.rs | 38 +- src/model/calibration.rs | 838 +++++++++++++++++++++++++++++++++++ src/model/catalog.rs | 18 + src/model/course.rs | 78 +++- src/model/course/fragment.rs | 108 +++++ src/model/item.rs | 9 +- src/model/layout.rs | 20 + src/model/seal.rs | 80 ++++ src/util.rs | 1 + src/util/citation.rs | 330 ++++++++++++++ 18 files changed, 2538 insertions(+), 108 deletions(-) create mode 100644 src/model/calibration.rs create mode 100644 src/util/citation.rs diff --git a/src/analysis/calibrate.rs b/src/analysis/calibrate.rs index e171085..7bd914a 100644 --- a/src/analysis/calibrate.rs +++ b/src/analysis/calibrate.rs @@ -28,18 +28,19 @@ use std::collections::{BTreeMap, BTreeSet}; use std::path::PathBuf; +use crate::SCHEMA_VERSION; use crate::assessment::AssessmentFile; -use crate::bank::BankFile; +use crate::calibration::{CalibrationFile, Measurement, MeasurementFile, MeasurementMeta}; use crate::catalog::Catalog; use crate::classical::{self, Analysis, ItemAnalysis, Thresholds}; use crate::date::Date; use crate::error::{Error, Result}; use crate::irt::{self, Fit}; use crate::item::{Calibration, IrtParams, Item, OptionStat, VariantCalibration}; +use crate::layout::Layout; use crate::responses::ResponseSet; use crate::store::Store; use crate::taxonomy::Flag; -use crate::yaml; /// What calibration would change about one item. #[derive(Debug, Clone)] @@ -615,60 +616,123 @@ fn diff_calibration(previous: Option<&Calibration>, next: &Calibration) -> Vec Result> { - // Group by file so each is read and written once. - let mut by_file: BTreeMap<&PathBuf, Vec<&Change>> = BTreeMap::new(); +/// Returns [`Error::Io`] on a write failure and [`Error::Yaml`] if the existing +/// store does not parse. +pub fn apply(layout: &Layout, plan: &Plan) -> Result { + let path = layout.calibration_file(); + let mut store = CalibrationFile::load(&path)?; + for change in &plan.changes { - by_file.entry(&change.path).or_default().push(change); + store + .items + .insert(change.uid.clone(), change.calibration.clone()); } - let mut written = Vec::new(); - for (path, changes) in by_file { - let mut bank: BankFile = yaml::read(path)?; - for change in changes { - // The uid is `bank::item`; match on the item part. - let item_id = change - .uid - .split_once("::") - .map(|(_, id)| id) - .unwrap_or(&change.uid); - let target = bank.items.iter_mut().find(|i| i.id == item_id); - match target { - Some(item) => item.calibration = Some(change.calibration.clone()), - None => { - return Err(Error::Unresolved { - kind: "item", - id: change.uid.clone(), - context: Some(format!( - "{} — the bank changed since the plan was built; re-run calibration", - path.display() - )), - }); - } - } - } - yaml::write(path, &bank)?; - written.push(path.clone()); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).map_err(|e| Error::io(parent, e))?; } - Ok(written) + store.save(&path)?; + Ok(path) +} + +/// Records what one administration measured, as a file that is never rewritten. +/// +/// The audit trail under the pooled store: these are the numbers one exam +/// produced, on a day, under a named model. Keeping them means the history +/// survives losing `data/`, which is ignored by git precisely because every row +/// of it carries a student. +/// +/// # Arguments +/// +/// * `layout` - the course layout. +/// * `administration` - the administration id, which names the file. +/// * `analysis` - the classical analysis of that administration. +/// * `catalog` - the loaded course, for the digests each item was measured +/// against. +/// * `record` - the assessment record, for the term and date. +/// +/// # Returns +/// +/// The two files written: one row per question, one row per question and +/// option. +/// +/// # Errors +/// +/// Returns [`Error::Usage`] when a record for this administration already +/// exists, since an administration happened once. +pub fn record_measurements( + layout: &Layout, + administration: &str, + analysis: &Analysis, + catalog: &Catalog, + record: Option<&AssessmentFile>, +) -> Result> { + let mut items = Vec::new(); + for item in &analysis.items { + let Some(uid) = &item.item_ref else { continue }; + let entry = catalog.get(uid); + items.push(Measurement { + item: uid.clone(), + number: item.number, + variant: item.variant.clone(), + stem_digest: entry.map(|e| e.item.stem_digest()), + n: item.n, + p_value: Some(round4(item.p_value)), + point_biserial: item.point_biserial.map(round4), + discrimination_index: item.discrimination_index.map(round4), + key: item.key.clone(), + option_stats: item + .options + .iter() + .map(|(id, o)| (id.clone(), o.to_option_stat())) + .collect(), + irt: None, + flags: item.flags.clone(), + }); + } + + let file = MeasurementFile { + schema_version: SCHEMA_VERSION.to_string(), + administration: MeasurementMeta { + id: administration.to_string(), + assessment: record + .map(|r| r.assessment.id.clone()) + .unwrap_or_else(|| administration.to_string()), + term: record.and_then(|r| r.assessment.term.clone()), + date: record.and_then(|r| r.assessment.date), + forms: record + .map(|r| r.forms.iter().map(|f| f.id.clone()).collect()) + .unwrap_or_default(), + // The cohort, taken as the largest per-item n: a student who + // skipped question 7 still sat the exam. + n_examinees: analysis.items.iter().map(|i| i.n).max().unwrap_or(0), + model: None, + generated: Some(Date::today()), + coursebank: Some(crate::VERSION.to_string()), + }, + items, + }; + + file.write_csv(&layout.measurements()) } /// Builds a plan for a single administration, from an in-memory analysis. diff --git a/src/authoring/jsonschema.rs b/src/authoring/jsonschema.rs index fd3c4fb..cd7f360 100644 --- a/src/authoring/jsonschema.rs +++ b/src/authoring/jsonschema.rs @@ -37,6 +37,10 @@ pub enum Kind { Lecture, /// `objectives/*.yaml`. Objective, + /// `analysis/calibration.yaml`. + Calibration, + /// `analysis/administrations/*.yaml`. + Measurements, /// `banks/*.yaml`. Bank, /// `assessments/*.yaml`. @@ -45,13 +49,15 @@ pub enum Kind { impl Kind { /// Every kind. - pub const ALL: [Kind; 6] = [ + pub const ALL: [Kind; 8] = [ Kind::Course, Kind::References, Kind::Lecture, Kind::Objective, Kind::Bank, Kind::Assessment, + Kind::Calibration, + Kind::Measurements, ]; /// The file name a schema is written to. @@ -61,6 +67,8 @@ impl Kind { Kind::References => "references.schema.json", Kind::Lecture => "lecture.schema.json", Kind::Objective => "objective.schema.json", + Kind::Calibration => "calibration.schema.json", + Kind::Measurements => "measurements.schema.json", Kind::Bank => "bank.schema.json", Kind::Assessment => "assessment.schema.json", } @@ -99,6 +107,8 @@ pub fn schema(kind: Kind) -> Value { Kind::References => references_schema(), Kind::Lecture => lecture_fragment_schema(), Kind::Objective => objective_fragment_schema(), + Kind::Calibration => calibration_file_schema(), + Kind::Measurements => measurements_file_schema(), Kind::Bank => bank_schema(), Kind::Assessment => assessment_schema(), } @@ -381,9 +391,12 @@ fn target_schema() -> Value { "order": { "type": "integer", "minimum": 1, - "description": "Position among the other targets of the same objective, low \ - first. Ordered within its objective rather than across the \ - course, so inserting one renumbers nothing outside its group." + "description": "Ignored since 2.0. An objective's targets are a set of question \ + templates, not steps in a sequence — they are not taught in \ + order and an exam samples from them — so a position asserts an \ + order that does not exist. Where one target depends on another, \ + say so with `prerequisites`. `coursebank migrate order` removes \ + this." }, "level_ceiling": level(), "prerequisites": string_array( @@ -412,8 +425,10 @@ fn objective_schema() -> Value { "order": { "type": "integer", "minimum": 1, - "description": "Position in teaching order, low first. Without it objectives \ - sort by id, which puts one before its own prerequisite." + "description": "Position in teaching order, low first. Derived since 2.0 from \ + the position of this objective in a lecture's `teaches` list; \ + an authored value still wins, and `coursebank migrate order` \ + removes them." }, "level_ceiling": level(), "prerequisites": string_array( @@ -1058,6 +1073,106 @@ fn option_history_schema() -> Value { }) } +/// The schema for `analysis/calibration.yaml`. +fn calibration_file_schema() -> Value { + json!({ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": format!("{BASE}/calibration.schema.json"), + "title": "coursebank calibration store", + "description": "The pooled statistics, by item id. Kept out of the banks: a bank's diff \ + should be a change of intent, not the output of a grading run. Committed \ + — every number here is a cohort aggregate, and there is no field for a \ + student.", + "type": "object", + "additionalProperties": false, + "properties": { + "schema_version": { "type": ["string", "number"] }, + "items": { + "type": "object", + "description": "Keyed by item id, which since 2.0 names the item course-wide and \ + carries no file name, so moving a question between banks does \ + not orphan its statistics.", + "additionalProperties": calibration_schema() + } + } + }) +} + +/// The schema for one file under `analysis/administrations/`. +fn measurements_file_schema() -> Value { + json!({ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": format!("{BASE}/measurements.schema.json"), + "title": "coursebank administration record", + "description": "What one administration measured. Written once and not rewritten, like \ + a seal: it records a thing that happened on a day. Cohort aggregates \ + only — no per-section or per-student breakdown, because a small cell \ + crossed with anything else stops being an aggregate.", + "type": "object", + "required": ["administration"], + "additionalProperties": false, + "properties": { + "schema_version": { "type": ["string", "number"] }, + "administration": { + "type": "object", + "required": ["id", "assessment", "n_examinees"], + "additionalProperties": false, + "properties": { + "id": { "type": "string" }, + "assessment": { "type": "string" }, + "term": { "type": "string" }, + "date": date("When it was given."), + "forms": string_array("The forms in play."), + "n_examinees": { + "type": "integer", + "minimum": 0, + "description": "The number to read before any of the others: a \ + point-biserial on twenty-seven students is a different \ + kind of claim than one on three hundred." + }, + "model": { "type": "string" }, + "generated": date("When the analysis was run."), + "coursebank": { "type": "string" } + } + }, + "items": { + "type": "array", + "items": { + "type": "object", + "required": ["item", "number", "n"], + "additionalProperties": false, + "properties": { + "item": { "type": "string" }, + "number": { "type": "integer", "minimum": 1 }, + "variant": { "type": "string" }, + "stem_digest": { "type": "string" }, + "n": { "type": "integer", "minimum": 0 }, + "p_value": proportion("Proportion correct on this administration."), + "point_biserial": { "type": "number", "minimum": -1.0, "maximum": 1.0 }, + "discrimination_index": { + "type": "number", "minimum": -1.0, "maximum": 1.0 + }, + "option_stats": { + "type": "object", + "additionalProperties": option_stat_schema() + }, + "irt": irt_schema(), + "flags": { + "type": "array", + "items": { + "type": "string", + "enum": strings( + &Flag::ALL.iter().map(|f| f.as_str()).collect::>() + ) + } + } + } + } + } + } + }) +} + /// The schema for a retirement record. fn retirement_schema() -> Value { json!({ @@ -1080,9 +1195,12 @@ fn item_identity_properties() -> Value { json!({ "id": { "type": "string", - "pattern": "^q-[a-z0-9]+(-[a-z0-9]+)*-[0-9]{3}$", - "description": "Item id, e.g. q-glycolysis-014. Stable forever: assessment records \ - and stored responses refer to it." + "pattern": "^q-[a-z0-9]+(-[a-z0-9]+)*$", + "description": "Item id, e.g. q-glycolysis-rate-limiting-step. Stable forever: \ + assessment records and stored responses refer to it, so renaming one \ + is a migration rather than an edit. A trailing counter is no longer \ + expected — it recorded when the item was written, which git knows — \ + but an id that still has one stays valid." }, "version": { "type": "integer", @@ -1149,7 +1267,6 @@ fn item_content_properties() -> Value { "prerequisites": string_array("Objective ids a student needs before this item."), "assets": { "type": "array", "items": asset_schema() }, "design": design_schema(), - "calibration": calibration_schema(), "review": review_schema(), "history": { "type": "array", diff --git a/src/cli.rs b/src/cli.rs index ea3e3f7..ce75579 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -137,6 +137,37 @@ pub(crate) enum MigrateCommand { #[arg(long)] dry_run: bool, }, + /// Drop the trailing counter from every item id. + /// + /// A rename, not a normalization: `q-x-001` and `q-x` are unrelated + /// strings, so banks, records, seals, and the response store are rewritten + /// in one pass or not at all. Two ids that would collide abort it. + Counters { + /// Show the renames, and write nothing. + #[arg(long)] + dry_run: bool, + }, + /// Replace the `order:` integers with ordered declarations. + /// + /// Objective order comes from each lecture's `teaches` list, target order + /// from a `targets:` list this writes onto each objective. Run + /// `migrate split` first — without `teaches`, objective order has no + /// source. + Order { + /// Show what would change, and write nothing. + #[arg(long)] + dry_run: bool, + }, + /// Turn citations written into `note:` fields into real fields. + /// + /// Journal, volume, pages, and DOI parsed out of the prose, with whatever + /// the note still says left in it. A field the entry already declares is + /// never overwritten; a disagreement is reported instead. + References { + /// Show what would change, and write nothing. + #[arg(long)] + dry_run: bool, + }, /// Fill in the stored `variant` column from the assessment records. /// /// Nothing in the tool needs it — a variant is derived from the placement @@ -724,6 +755,14 @@ pub(crate) enum AnalyzeCommand { /// Pool every stored administration of this assessment. #[arg(long)] pooled: bool, + /// Also write the administration record under analysis/. + /// + /// Two CSVs, one row per question and one per question-and-option, + /// written once and never rewritten. Cohort aggregates only, so unlike + /// the response data they are meant to be committed — which is what + /// keeps the history when `data/` rotates. + #[arg(long = "record")] + record: bool, }, /// Fit an IRT model. Irt { diff --git a/src/commands/analysis.rs b/src/commands/analysis.rs index e1d1554..560251f 100644 --- a/src/commands/analysis.rs +++ b/src/commands/analysis.rs @@ -294,12 +294,46 @@ pub(crate) fn analyze(cli: &Cli, sub: &AnalyzeCommand) -> Result { let store = Store::open(catalog.layout.data())?; match sub { - AnalyzeCommand::Items { id, pooled } => { + AnalyzeCommand::Items { + id, + pooled, + record: write_record, + } => { let record = load_record(&catalog, id)?; let set = responses_for(&store, &catalog, &record, *pooled)?; let analysis = classical::analyze(&set, &Thresholds::default(), Some(&record), Some(&catalog)); + if *write_record { + // The full administration id, not the assessment id: `e1` is + // given again next year, and a record named after it would + // collide with this cohort's — which `write_csv` would report + // as "already recorded" when it is a different exam entirely. + let administration = coursebank::responses::administration_id( + &catalog.course.course.code, + record + .assessment + .term + .as_deref() + .unwrap_or(&catalog.course.course.term), + &record.assessment.id, + ); + let written = calibrate::record_measurements( + &catalog.layout, + &administration, + &analysis, + &catalog, + Some(&record), + )?; + for path in &written { + println!("wrote {}", path.display()); + } + println!( + "\nCohort aggregates only, so these are committed. Review with\n git diff \ + analysis/\n" + ); + } + for w in &analysis.warnings { println!("! {w}\n"); } @@ -463,17 +497,20 @@ pub(crate) fn calibrate(cli: &Cli, args: &CalibrateArgs) -> Result { } if !args.apply { println!( - "Nothing written. Re-run with --apply to write these {} change(s) into the bank \ - files, then review the git diff.", + "Nothing written. Re-run with --apply to write these {} change(s) into \ + analysis/calibration.yaml, then review the git diff.", plan.changes.len() ); return Ok(Outcome::Ok); } - for path in calibrate::apply(&plan)? { - println!("updated {}", path.display()); - } - println!("\nReview the diff before committing: git diff banks/"); + let path = calibrate::apply(&catalog.layout, &plan)?; + println!("updated {}", path.display()); + println!( + "\nReview the diff before committing: git diff analysis/\n\nThe banks are untouched. \ + Statistics are cohort aggregates with no student in\n them, which is why analysis/ is \ + committed and data/ is not." + ); Ok(Outcome::Ok) } diff --git a/src/commands/project.rs b/src/commands/project.rs index 4235799..03088f7 100644 --- a/src/commands/project.rs +++ b/src/commands/project.rs @@ -35,10 +35,15 @@ use crate::helpers::{load, truncate}; /// The `.gitignore` written by `init`. pub(crate) const GITIGNORE: &str = "\ -# Generated output: exports, rendered exams, reports. +# Generated output: exports, rendered exams, reports. A student report carries +# names, so it belongs here rather than in the repository. build/ reports/ +# Response data: every row carries a student. The statistics derived from it are +# cohort aggregates and live in analysis/, which is committed on purpose. +data/ + # Typst and PDF artifacts. *.pdf @@ -109,9 +114,105 @@ pub(crate) fn migrate(cli: &Cli, sub: &MigrateCommand) -> Result { MigrateCommand::Options { dry_run } => migrate_options(cli, *dry_run), MigrateCommand::Stems { dry_run } => migrate_stems(cli, *dry_run), MigrateCommand::Variants { dry_run } => migrate_variants(cli, *dry_run), + MigrateCommand::References { dry_run } => migrate_references(cli, *dry_run), + MigrateCommand::Order { dry_run } => migrate_order(cli, *dry_run), + MigrateCommand::Counters { dry_run } => migrate_counters(cli, *dry_run), } } +/// Drops the trailing counter from every item id. +fn migrate_counters(cli: &Cli, dry_run: bool) -> Result { + let (rename, touched) = migrate::counters(&cli.course, !dry_run)?; + if rename.is_empty() { + println!("nothing to do: no item id ends in a counter"); + return Ok(Outcome::Ok); + } + + for (old, new) in &rename { + println!(" {old} -> {new}"); + } + println!(); + for (path, n) in &touched { + println!(" {:<44} {n:>5} reference(s)", path.display()); + } + if dry_run { + println!("\nnothing written"); + return Ok(Outcome::Ok); + } + + let sealed = touched + .iter() + .filter(|(p, _)| p.starts_with("seals")) + .count(); + println!( + "\nrenamed {} item(s) across {} file(s)", + rename.len(), + touched.len() + ); + if sealed > 0 { + println!( + "\n{sealed} seal(s) were rewritten. The ids are inside the digest, so each one was \ + recomputed\n and the previous digest recorded under `superseded_digests`. A seal \ + that has been\n rewritten says so rather than looking untouched." + ); + } + println!( + "\nNext:\n coursebank validate\n coursebank seal verify\n coursebank analyze items \ + --all # the join key moved; check the data still lands" + ); + Ok(Outcome::Ok) +} + +/// Replaces the order integers with ordered declarations. +fn migrate_order(cli: &Cli, dry_run: bool) -> Result { + let touched = migrate::order(&cli.course, !dry_run)?; + if touched.is_empty() { + println!("nothing to do: no `order:` left to derive"); + return Ok(Outcome::Ok); + } + let total: usize = touched.iter().map(|(_, n)| n).sum(); + for (path, n) in &touched { + println!(" {:<44} {n:>5} order(s) dropped", path.display()); + } + if dry_run { + println!("\nnothing written"); + return Ok(Outcome::Ok); + } + println!( + "\nrewrote {} file(s), {total} integer(s) gone\n\nNext:\n coursebank validate\n \ + coursebank lecture objectives L1.2 # check the order still reads right", + touched.len() + ); + Ok(Outcome::Ok) +} + +/// Takes the citations out of the notes and puts them in fields. +fn migrate_references(cli: &Cli, dry_run: bool) -> Result { + let (touched, notes) = migrate::references(&cli.course, !dry_run)?; + + for (path, n) in &touched { + println!(" {:<44} {n:>5} note(s) taken apart", path.display()); + } + for note in ¬es { + println!(" note: {note}"); + } + if touched.is_empty() { + println!("nothing to do: no note is carrying a citation"); + return Ok(Outcome::Ok); + } + if dry_run { + println!("\nnothing written"); + return Ok(Outcome::Ok); + } + println!( + "\nrewrote {} file(s)\n\nNext:\n coursebank validate\n coursebank references list\n\n\ + An issue number is never inferred, not even from a DOI that encodes one, so add those \ + by hand.", + touched.len() + ); + Ok(Outcome::Ok) +} + /// Fills in the stored variant column. fn migrate_variants(cli: &Cli, dry_run: bool) -> Result { let touched = migrate::store_variants(&cli.course, !dry_run)?; @@ -590,6 +691,10 @@ pub(crate) fn validate(cli: &Cli) -> Result { let seals = coursebank::seal::SealFile::load_all(&catalog.layout.seals())?; all.extend(catalog.validate_seals(&seals)); + // The statistics are kept in a different file from the questions they + // describe, so the link between them is worth checking rather than assuming. + all.extend(catalog.calibration.validate(&catalog)); + if all.is_empty() { if !cli.quiet { println!( diff --git a/src/lib.rs b/src/lib.rs index d9d6294..f8a485b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -106,9 +106,11 @@ pub mod migrate; pub mod model; pub mod util; -pub use util::{date, hash, markup, rng, yaml, zipfile}; +pub use util::{citation, date, hash, markup, rng, yaml, zipfile}; -pub use model::{assessment, bank, catalog, course, history, item, layout, seal, taxonomy}; +pub use model::{ + assessment, bank, calibration, catalog, course, history, item, layout, seal, taxonomy, +}; pub use course::fragment; diff --git a/src/migrate.rs b/src/migrate.rs index bf8b3b4..4b901fd 100644 --- a/src/migrate.rs +++ b/src/migrate.rs @@ -19,6 +19,9 @@ //! | [`options_plan`] and [`apply_options`] | option letters into names | a letter is a position, not an identity | //! | [`stems`] | drops `version:` and `history:` | a stem's text is its identity | //! | [`store_variants`] | fills the stored `variant` column | so other readers can group by option set | +//! | [`references`] | prose notes into citation fields | a DOI in a note is a link nobody can follow | +//! | [`order`] | removes the `order:` integers | objective order is derived, and targets have none | +//! | [`counters`] | drops the `-001` from item ids | a per-bank sequence number is not part of a name | //! //! # What the option migration does not touch //! @@ -96,7 +99,7 @@ pub struct Plan { impl Plan { /// The number of lines each planned file holds. - pub fn lines(&self) -> Vec<(PathBuf, usize)> { + pub fn lines(&self) -> Report { self.files .iter() .map(|f| (f.path.clone(), f.text.lines().count())) @@ -602,7 +605,7 @@ fn unqualify_line(line: &str) -> String { /// /// Propagates read and write failures, including /// [`Error::FeatureDisabled`] when the store is Parquet and the feature is off. -pub fn store_ids(root: &Path, write: bool) -> Result> { +pub fn store_ids(root: &Path, write: bool) -> Result { let layout = Layout::new(root); let store = Store::open(layout.data())?; let mut out = Vec::new(); @@ -629,6 +632,15 @@ pub fn store_ids(root: &Path, write: bool) -> Result> { Ok(out) } +/// One file a migration changed, and how many things in it changed. +pub type Changed = (PathBuf, usize); + +/// What a migration changed, file by file. +pub type Report = Vec; + +/// Old id to new id, for the migrations that rename things. +pub type Renames = BTreeMap; + /// The per-item map from a pre-2.0 option letter to the name that replaces it. pub type OptionMap = BTreeMap>; @@ -844,7 +856,7 @@ pub fn options_plan(root: &Path) -> Result<(OptionMap, Vec)> { /// # Errors /// /// Propagates read and write failures. -pub fn apply_options(root: &Path, map: &OptionMap, write: bool) -> Result> { +pub fn apply_options(root: &Path, map: &OptionMap, write: bool) -> Result { let layout = Layout::new(root); let mut out = Vec::new(); @@ -1132,7 +1144,7 @@ fn key_at_word(line: &str, key: &str) -> Option { /// # Errors /// /// Propagates read and write failures. -pub fn stems(root: &Path, write: bool) -> Result> { +pub fn stems(root: &Path, write: bool) -> Result { let layout = Layout::new(root); let mut out = Vec::new(); @@ -1221,7 +1233,7 @@ fn is_key(trimmed: &str, key: &str) -> bool { /// # Errors /// /// Propagates catalog, record, and store failures. -pub fn store_variants(root: &Path, write: bool) -> Result> { +pub fn store_variants(root: &Path, write: bool) -> Result { let layout = Layout::new(root); let catalog = crate::catalog::Catalog::load(root)?; @@ -1265,6 +1277,475 @@ pub fn store_variants(root: &Path, write: bool) -> Result> Ok(out) } +/// Drops the trailing counter from every item id. +/// +/// `q-fastq-quality-line-001` becomes `q-fastq-quality-line`. The counter was a +/// per-bank sequence number, which says when an item was written — something git +/// knows — and collides with another bank's numbering the moment an item moves. +/// +/// This is a rename, not a normalization, and that distinction decides how it +/// has to be done. Stripping `bank::` from an id recovered a name that was +/// already inside it, so an old reference could simply be read as the new one. +/// A counter carries no such fallback: `q-x-001` and `q-x` are two unrelated +/// strings, and every file that names one has to be rewritten in the same pass +/// or the join to four terms of response data quietly breaks. +/// +/// So: banks, assessment records, seals, and the response store, or nothing. +/// Two ids that would collide after stripping abort the whole migration rather +/// than merging two questions into one. +/// +/// # Arguments +/// +/// * `root` - the course directory. +/// * `write` - whether to write, or only to count. +/// +/// # Returns +/// +/// The renames, and one entry per file that changes with how many references it +/// holds. +/// +/// # Errors +/// +/// Returns [`Error::Invalid`] when two ids would collide, listing both, and +/// propagates load and write failures. +pub fn counters(root: &Path, write: bool) -> Result<(Renames, Report)> { + let layout = Layout::new(root); + let catalog = crate::catalog::Catalog::load(root)?; + + let mut rename: Renames = BTreeMap::new(); + let mut taken: Renames = BTreeMap::new(); + let mut collisions = Vec::new(); + for entry in &catalog.entries { + let Some(bare) = strip_counter(&entry.uid) else { + taken.insert(entry.uid.clone(), entry.uid.clone()); + continue; + }; + if let Some(first) = taken.get(&bare) { + collisions.push(format!( + "`{}` and `{first}` would both become `{bare}`. Give one of them a name that \ + says what it asks rather than when it was written.", + entry.uid + )); + continue; + } + taken.insert(bare.clone(), entry.uid.clone()); + rename.insert(entry.uid.clone(), bare); + } + if !collisions.is_empty() { + return Err(Error::Invalid(collisions)); + } + if rename.is_empty() { + return Ok((rename, Vec::new())); + } + + let mut touched = Vec::new(); + + // Banks: the definition, and any `supersedes` pointing at one. + for path in yaml::list_yaml(&layout.banks())? { + let text = std::fs::read_to_string(&path).map_err(|e| Error::io(&path, e))?; + let (out, n) = rename_ids(&text, &rename, &["- id", "supersedes"]); + if n > 0 { + if write { + yaml::write_text(&path, &out)?; + } + touched.push((relative(root, &path), n)); + } + } + + // Records: the `item:` on every placement. + for path in yaml::list_yaml(&layout.assessments())? { + let text = std::fs::read_to_string(&path).map_err(|e| Error::io(&path, e))?; + let (out, n) = rename_ids(&text, &rename, &["item"]); + if n > 0 { + if write { + yaml::write_text(&path, &out)?; + } + touched.push((relative(root, &path), n)); + } + } + + // Seals go through the model, not the text, because the ids are inside the + // digest and a seal that has been rewritten has to say so. + for path in yaml::list_yaml(&layout.seals())? { + let mut seal = crate::seal::SealFile::load(&path)?; + let n = seal.rename_items(&rename); + if n > 0 { + if write { + seal.save(&path)?; + } + touched.push((relative(root, &path), n)); + } + } + + // The store, where the id is the join key. + let store = Store::open(layout.data())?; + for path in store.files()? { + let mut rows = store::read_flat(&path)?; + let mut n = 0; + for row in &mut rows { + if let Some(new) = rename.get(&row.item_ref) { + row.item_ref = new.clone(); + n += 1; + } + } + if n > 0 { + if write { + store::write_flat(&path, &rows)?; + } + touched.push((relative(root, &path), n)); + } + } + + Ok((rename, touched)) +} + +/// An id with a trailing `-123` removed, or `None` when it has none. +fn strip_counter(id: &str) -> Option { + let (head, tail) = id.rsplit_once('-')?; + if head.is_empty() || tail.is_empty() || !tail.chars().all(|c| c.is_ascii_digit()) { + return None; + } + Some(head.to_string()) +} + +/// Rewrites the value of the named keys wherever it is an id being renamed. +/// +/// Keyed on the field name so that an id appearing in prose — a rationale that +/// mentions the item it replaced, a note — is left alone. A rename that edited +/// every matching string in the file would also edit the sentences about it. +fn rename_ids(text: &str, rename: &Renames, keys: &[&str]) -> (String, usize) { + let mut out: Vec = Vec::new(); + let mut count = 0; + + for line in text.lines() { + let mut replaced = None; + for key in keys { + let Some(at) = field_value(line, key) else { + continue; + }; + let value = &line[at.0..at.1]; + // Canonicalized before the lookup, so a record that still carries a + // pre-2.0 `bank::` qualifier is renamed rather than skipped. Left + // alone it would keep pointing at an id the bank no longer has. + let Some(new) = rename.get(crate::item::canonical_id(value)) else { + continue; + }; + replaced = Some(format!("{}{new}{}", &line[..at.0], &line[at.1..])); + count += 1; + break; + } + out.push(replaced.unwrap_or_else(|| line.to_string())); + } + + let mut joined = out.join("\n"); + if text.ends_with('\n') { + joined.push('\n'); + } + (joined, count) +} + +/// The byte range of a `key: value` scalar on one line, quotes excluded. +fn field_value(line: &str, key: &str) -> Option<(usize, usize)> { + let needle = format!("{key}:"); + let at = line.find(&needle)?; + let before = &line[..at]; + if before.chars().next_back().is_some_and(|c| { + c.is_ascii_alphanumeric() || c == '_' || (c == '-' && !key.starts_with('-')) + }) { + return None; + } + let rest = &line[at + needle.len()..]; + let lead = rest.len() - rest.trim_start().len(); + let value = rest.trim_start(); + let quoted = value.starts_with(['"', '\'']); + let start = at + needle.len() + lead + usize::from(quoted); + let inner = &line[start..]; + let end = inner + .find(|c: char| { + c == '"' || c == '\'' || c == ',' || c == '}' || c == ']' || c.is_whitespace() + }) + .map(|n| start + n) + .unwrap_or(line.len()); + (end > start).then_some((start, end)) +} + +/// Removes the hand-kept `order:` integers. +/// +/// Nothing is written in their place. An objective's teaching order is its +/// position in a lecture's `teaches` list, which `migrate split` already +/// produced. A target has no order at all — an objective's targets are a set of +/// question templates, sampled from rather than worked through — so the integer +/// was asserting a sequence that was never taught. Where a list has to be +/// printed, [`crate::course::CourseFile::targets`] orders it by ceiling. +/// +/// Two hundred and four integers in a real course, each of which had to be +/// bumped by hand when a target was inserted in the middle. +/// +/// # Arguments +/// +/// * `root` - the course directory. +/// * `write` - whether to write, or only to count. +/// +/// # Returns +/// +/// One entry per file that changes, with how many `order:` lines it loses. +/// +/// # Errors +/// +/// Propagates catalog and write failures, and refuses when the lectures do not +/// declare what they teach, since objective order would then have no source. +pub fn order(root: &Path, write: bool) -> Result { + let layout = Layout::new(root); + let course = CourseFile::load_dir(root)?; + + if course.lectures.values().all(|l| l.teaches.is_empty()) + && !course.learning_objectives.is_empty() + { + return Err(Error::usage( + "no lecture declares what it teaches, so objective order would have nothing to come from. Run `coursebank migrate split` first." + .to_string(), + )); + } + + let mut out = Vec::new(); + let mut files = vec![layout.course_file()]; + files.extend(yaml::list_yaml(&layout.objectives())?); + for path in files { + if !path.is_file() { + continue; + } + let text = std::fs::read_to_string(&path).map_err(|e| Error::io(&path, e))?; + let (rewritten, dropped) = rewrite_order(&text); + if rewritten == text { + continue; + } + if write { + yaml::write_text(&path, &rewritten)?; + } + out.push((relative(root, &path), dropped)); + } + Ok(out) +} + +/// Removes every `order:` line. +/// +/// Nothing replaces them. An objective's position comes from where its lecture +/// lists it in `teaches`, and a target has no position to derive: an +/// objective's targets are a set of question templates rather than steps in a +/// sequence, so the integer was asserting an order that was never taught. +fn rewrite_order(text: &str) -> (String, usize) { + let mut out: Vec<&str> = Vec::new(); + let mut dropped = 0; + + for line in text.lines() { + if key_at(line, 4).as_deref() == Some("order") { + dropped += 1; + continue; + } + out.push(line); + } + + let mut joined = out.join("\n"); + if text.ends_with('\n') { + joined.push('\n'); + } + (joined, dropped) +} + +/// Turns the citations written into `note:` fields into real fields. +/// +/// A hand-built bibliography collects entries whose journal, volume, pages, and +/// DOI are all sitting in the one field that means "anything else worth saying". +/// Nothing can use them there: a reading list cannot link a DOI it cannot see, +/// and an export to Hayagriva or BibTeX has no journal to put in `parent` or +/// `journal`. +/// +/// Textual, like the rest of this module, and for a reason specific to this +/// file: a bibliography is usually ordered the way its author thinks about it — +/// books, then the papers that matter — and the model holds it in a map, so a +/// serde round trip would alphabetize all of it to change twenty entries. +/// +/// A field the entry already declares is never overwritten. Where the note +/// disagrees with it, the note's version is left in place for a person to look +/// at rather than silently replacing something that was typed deliberately. +/// +/// # Arguments +/// +/// * `root` - the course directory. +/// * `write` - whether to write, or only to count. +/// +/// # Returns +/// +/// One entry per file that changes, with how many notes were taken apart, and +/// one message per disagreement found. +/// +/// # Errors +/// +/// Propagates read and write failures. +pub fn references(root: &Path, write: bool) -> Result<(Report, Vec)> { + let layout = Layout::new(root); + let mut touched = Vec::new(); + let mut notes = Vec::new(); + + // Either file may hold the bibliography: its own, or the course file that + // has not been split yet. + for path in [layout.references_file(), layout.course_file()] { + if !path.is_file() { + continue; + } + let text = std::fs::read_to_string(&path).map_err(|e| Error::io(&path, e))?; + let (rewritten, count, said) = expand_notes(&text); + notes.extend(said); + if count > 0 { + if write { + yaml::write_text(&path, &rewritten)?; + } + touched.push((relative(root, &path), count)); + } + } + Ok((touched, notes)) +} + +/// Replaces each reference's `note:` line with the fields it was carrying. +fn expand_notes(text: &str) -> (String, usize, Vec) { + let lines: Vec = text.lines().map(str::to_string).collect(); + let Some(section) = blocks(&lines, 0) + .into_iter() + .find(|b| b.key == "references") + else { + return (text.to_string(), 0, Vec::new()); + }; + + // Which lines belong to which entry, so a note is parsed with its own year + // and checked against its own fields. + let mut rewritten: BTreeMap> = BTreeMap::new(); + let mut said = Vec::new(); + let mut count = 0; + + let start = lines.len() - section.lines.len(); + for entry in blocks(§ion.body(), 2) { + let offset = lines + .iter() + .enumerate() + .skip(start) + .find(|(_, l)| **l == entry.lines[0]) + .map(|(i, _)| i); + let Some(offset) = offset else { continue }; + + let Some((note_at, note)) = entry + .lines + .iter() + .enumerate() + .find_map(|(i, l)| scalar(l, 4, "note").map(|v| (i, v))) + else { + continue; + }; + + let year = entry + .lines + .iter() + .find_map(|l| scalar(l, 4, "year")) + .and_then(|v| v.parse::().ok()); + let parsed = crate::citation::parse(¬e, year); + if parsed.container.is_none() && parsed.doi.is_none() { + continue; + } + + let declared = |field: &str| entry.lines.iter().any(|l| scalar(l, 4, field).is_some()); + let mut out: Vec = Vec::new(); + for (field, value, quote) in [ + ("container", parsed.container.clone(), false), + ("volume", parsed.volume.clone(), true), + ("pages", parsed.pages.clone(), true), + ("doi", parsed.doi.clone(), true), + ] { + let Some(value) = value else { continue }; + if declared(field) { + said.push(format!( + "`{}` already declares {field}; the note's `{value}` was left in place", + entry.key + )); + continue; + } + out.push(format!(" {field}: {}", render(&value, quote))); + } + if let Some(note) = &parsed.note { + out.push(format!(" note: {}", render(note, true))); + } + + // A note whose every part was already declared is not a change. + if out.is_empty() { + continue; + } + if said + .iter() + .any(|m| m.starts_with(&format!("`{}`", entry.key))) + && parsed.note.is_some() + && out.len() == 1 + { + continue; + } + rewritten.insert(offset + note_at, out); + count += 1; + } + + if count == 0 { + return (text.to_string(), 0, said); + } + + let mut out: Vec = Vec::new(); + for (i, line) in lines.iter().enumerate() { + match rewritten.get(&i) { + Some(replacement) => out.extend(replacement.clone()), + None => out.push(line.clone()), + } + } + let mut joined = out.join("\n"); + if text.ends_with('\n') { + joined.push('\n'); + } + (joined, count, said) +} + +/// The value of a `key: value` line at an exact indentation, unquoted. +fn scalar(line: &str, indent: usize, key: &str) -> Option { + if key_at(line, indent).as_deref() != Some(key) { + return None; + } + let value = line[indent + key.len() + 1..].trim(); + if value.is_empty() { + return None; + } + for quote in ['\'', '"'] { + if let Some(inner) = value + .strip_prefix(quote) + .and_then(|v| v.strip_suffix(quote)) + { + return Some(inner.replace("''", "'")); + } + } + Some(value.to_string()) +} + +/// A YAML scalar, quoted when it has to be. +/// +/// Numbers are quoted whether they need it or not: `volume: 48` reads back as an +/// integer and the field is a string, so the file would stop loading. +fn render(value: &str, quote: bool) -> String { + let risky = quote + || value.contains(": ") + || value.ends_with(':') + || value.contains(" #") + || value.starts_with([ + '[', '{', '&', '*', '!', '|', '>', '%', '@', '`', '\'', '"', '-', + ]); + if risky { + format!("'{}'", value.replace('\'', "''")) + } else { + value.to_string() + } +} + /// One block of YAML: a key, the lines under it, and the comments above it. #[derive(Debug, Clone, Default)] struct Block { @@ -1952,6 +2433,154 @@ items: ); } + #[test] + fn the_order_integers_are_dropped_and_nothing_replaces_them() { + let text = r#"learning_objectives: + lo-x: + text: 'Read the formats.' + unit: u1 + order: 2 + level_ceiling: 2 + +learning_targets: + t-a: + text: 'First.' + objective: lo-x + order: 1 + level_ceiling: 1 + t-b: + text: 'Second.' + objective: lo-x + order: 2 + level_ceiling: 2 +"#; + let (out, dropped) = rewrite_order(text); + assert_eq!(dropped, 3, "one on the objective, one on each target"); + assert!(!out.contains("order:"), "{out}"); + // Nothing is written in their place: the objective's position comes + // from its lecture's `teaches`, and a target has no position. Matched + // on the inserted form rather than on `targets:`, which is a substring + // of the `learning_targets:` section header. + assert!(!out.contains("targets: ["), "{out}"); + // Everything else is untouched, prose and ceilings included. + assert!(out.contains("text: 'Read the formats.'"), "{out}"); + assert!(out.contains("level_ceiling: 2"), "{out}"); + assert!(out.contains(" t-a:"), "{out}"); + + // Idempotent. + let (again, dropped) = rewrite_order(&out); + assert_eq!(dropped, 0); + assert_eq!(again, out); + } + + #[test] + fn a_note_becomes_the_fields_it_was_carrying() { + let text = r#"references: + altschul1997gapped: + label: GBLAST97 + kind: article + role: supplemental + title: 'Gapped BLAST and PSI-BLAST' + year: 1997 + note: 'Nucleic Acids Res 25:3389-3402. doi:10.1093/nar/25.17.3389' + brown2013next: + label: BSM + kind: book + title: Next-generation DNA sequencing informatics + year: 2013 +"#; + let (out, count, said) = expand_notes(text); + assert_eq!(count, 1); + assert!(said.is_empty(), "{said:?}"); + assert!(out.contains(" container: Nucleic Acids Res"), "{out}"); + assert!(out.contains(" volume: '25'"), "{out}"); + assert!(out.contains(" pages: '3389-3402'"), "{out}"); + assert!(out.contains(" doi: '10.1093/nar/25.17.3389'"), "{out}"); + // Nothing left of the note, and nothing invented to replace it. + assert!(!out.contains("note:"), "{out}"); + assert!(!out.contains("issue:"), "{out}"); + // The book had no note and is untouched, label and all. + assert!(out.contains(" label: BSM"), "{out}"); + assert!(out.contains(" brown2013next:"), "{out}"); + } + + #[test] + fn a_field_already_declared_is_never_overwritten() { + let text = r#"references: + steinegger2017mmseqs2: + kind: article + title: MMseqs2 + year: 2017 + note: 'Nat Biotechnol 35:1026-1028. doi:10.1038/nbt.3988' + doi: '10.1038/nbt.3988' +"#; + let (out, _, said) = expand_notes(text); + assert!( + said.iter().any(|m| m.contains("already declares doi")), + "{said:?}" + ); + // One doi line, the one that was typed deliberately. + assert_eq!(out.matches("doi:").count(), 1, "{out}"); + assert!(out.contains(" container: Nat Biotechnol"), "{out}"); + } + + #[test] + fn a_trailing_counter_is_recognized_and_nothing_else_is() { + assert_eq!( + strip_counter("q-fastq-quality-line-001").as_deref(), + Some("q-fastq-quality-line") + ); + assert_eq!( + strip_counter("q-blast-seed-14").as_deref(), + Some("q-blast-seed") + ); + // Already bare. + assert_eq!(strip_counter("q-fastq-quality-line"), None); + // A number that is part of the name, not a counter after it. + assert_eq!(strip_counter("q-fastqc-3prime-decay"), None); + assert_eq!(strip_counter("q-001"), Some("q".to_string())); + assert_eq!(strip_counter("q-x-"), None); + } + + #[test] + fn a_rename_touches_the_named_fields_and_leaves_prose_alone() { + let mut rename = BTreeMap::new(); + rename.insert("q-x-001".to_string(), "q-x".to_string()); + + let bank = r#"items: + - id: q-x-001 + supersedes: q-x-001 + stem: Which line? + design: + rationale: >- + Replaces q-x-001, which had two defensible answers. +"#; + let (out, n) = rename_ids(bank, &rename, &["- id", "supersedes"]); + assert_eq!(n, 2, "the id and the supersedes, not the sentence"); + assert!(out.contains(" - id: q-x\n"), "{out}"); + assert!(out.contains(" supersedes: q-x\n"), "{out}"); + // The rationale is prose about the item; editing it would rewrite the + // sentence as well as the reference. + assert!(out.contains("Replaces q-x-001, which had"), "{out}"); + } + + #[test] + fn a_record_that_never_dropped_its_bank_prefix_is_still_renamed() { + let mut rename = BTreeMap::new(); + rename.insert("q-x-001".to_string(), "q-x".to_string()); + + let flow = " - { number: 1, item: \"b-1-2::q-x-001\", key: [o-a] }\n"; + let (out, n) = rename_ids(flow, &rename, &["item"]); + assert_eq!(n, 1); + // Left alone it would point at an id the bank no longer has. + assert!(out.contains("item: \"q-x\""), "{out}"); + + let block = " item: q-x-001\n"; + let (out, n) = rename_ids(block, &rename, &["item"]); + assert_eq!(n, 1); + assert_eq!(out, " item: q-x\n"); + } + #[test] fn lecture_slugs_match_the_house_naming() { assert_eq!(lecture_slug("L1.2"), "l-1-2"); diff --git a/src/model.rs b/src/model.rs index ae66e04..f35f594 100644 --- a/src/model.rs +++ b/src/model.rs @@ -28,6 +28,7 @@ pub mod assessment; pub mod bank; +pub mod calibration; pub mod catalog; pub mod course; pub mod history; diff --git a/src/model/bank.rs b/src/model/bank.rs index b9456c3..19b5bd8 100644 --- a/src/model/bank.rs +++ b/src/model/bank.rs @@ -530,38 +530,12 @@ fn validate_item( } // --- calibration plausibility ------ - if let Some(c) = &it.calibration { - if let Some(p) = c.p_value { - if !(0.0..=1.0).contains(&p) { - issues.push(format!( - "calibration.p_value must be between 0 and 1, got {p}" - )); - } - } - if let Some(r) = c.point_biserial { - if !(-1.0..=1.0).contains(&r) { - issues.push(format!( - "calibration.point_biserial must be between -1 and 1, got {r}" - )); - } - } - for option in c.option_stats.keys() { - if it.option(option).is_none() { - issues.push(format!( - "calibration.option_stats has `{option}`, which is not an option of this item" - )); - } - } - if let Some(irt) = &c.irt { - if irt.a <= 0.0 { - issues.push(format!("calibration.irt.a must be positive, got {}", irt.a)); - } - if let Some(cp) = irt.c { - if !(0.0..1.0).contains(&cp) { - issues.push(format!("calibration.irt.c must be in [0, 1), got {cp}")); - } - } - } + if it.calibration.is_some() { + issues.push( + "has a `calibration:` block, but statistics live in analysis/calibration.yaml since \ + 2.0. A bank's diff should be a change of intent, not the output of a grading run." + .to_string(), + ); } // --- retirement ----- diff --git a/src/model/calibration.rs b/src/model/calibration.rs new file mode 100644 index 0000000..114cf3d --- /dev/null +++ b/src/model/calibration.rs @@ -0,0 +1,838 @@ +// SPDX-License-Identifier: Prosperity-3.0.0 +// Copyright Scientific Computing Studio +// Source: https://git.scient.ing/education/coursebank + +//! Where the statistics live, which is not in the bank. +//! +//! A bank file is a reviewed artifact: someone wrote the question, someone +//! argued about the distractors, and the diff on it should be a change of +//! intent. Statistics are neither reviewed nor intended — they are what +//! happened — and writing them back into the bank means every grading run +//! produces a diff on a file whose history is supposed to be about wording. +//! +//! So the evidence lives in `analysis/`, in two kinds of file: +//! +//! | File | Format | Rewritten? | Holds | +//! |:--|:--|:--|:--| +//! | `analysis/administrations/-items.csv` | CSV | never | one row per question | +//! | `analysis/administrations/-options.csv` | CSV | never | one row per question and option | +//! | `analysis/calibration.yaml` | YAML | by `calibrate` | the pooled per-item view | +//! +//! The format follows the shape. An administration record is a table — fixed +//! columns, one row per question, machine-written, never hand-edited — so it is +//! CSV: one line per item rather than fifteen, which diffs better, and it loads +//! straight into pandas or DuckDB, which is much of the point of committing it. +//! The pooled view is not a table. It is three levels deep, item to variant to +//! option history, with fitted parameters and variable-length lists, and as CSV +//! that would be three files joined by keys — a relational schema for the one +//! file a person actually reads in a pull request. That stays YAML. +//! +//! Neither is Parquet, and the reason is the review workflow: a binary file +//! shows nothing in a diff and cannot be merged. Parquet is right for `data/` +//! precisely because that is bulk, ignored, and never reviewed. +//! +//! An administration file is written once and not touched again, for the same +//! reason a seal is not: it is a record of a thing that happened on a day. The +//! calibration file is the accepted rollup — what `lint` compares your +//! predictions against, and what a report reads — and `calibrate` proposes +//! changes to it as a diff you review before committing. +//! +//! Both are meant to be committed. Neither can carry student data, and that is +//! a property of the types rather than a promise: there is no field for a +//! student key, an identifier, a section, or an ability estimate, and the +//! structures reject unknown keys, so a file carrying one fails to load rather +//! than being quietly accepted. Everything per-person stays in `data/`, which +//! is what your `.gitignore` is for. +//! +//! # Linking back to the bank +//! +//! By item id, which since 2.0 names the item course-wide and has no file name +//! in it, and by variant digest, which says which option set the numbers +//! describe. A record also carries the stem digest it was measured against, so +//! [`CalibrationFile::validate`] can say that an item has been reworded since — +//! the statistics then describe a question that no longer exists under that id. + +use std::collections::BTreeMap; +use std::path::{Path, PathBuf}; + +use serde::{Deserialize, Serialize}; + +use crate::course::SCHEMA_VERSION; +use crate::date::Date; +use crate::error::{Error, Result}; +use crate::item::{Calibration, IrtModel, IrtParams, OptionStat}; +use crate::taxonomy::Flag; +use crate::yaml; + +/// The file name of the pooled calibration store, under `analysis/`. +pub const CALIBRATION_FILE: &str = "calibration.yaml"; + +/// The pooled per-item statistics: `analysis/calibration.yaml`. +/// +/// Keyed by item id. This is the file `calibrate` rewrites and the one +/// everything else reads; [`crate::catalog::Catalog::load`] fills each item's +/// in-memory calibration from it, so nothing downstream has to know where the +/// numbers came from. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct CalibrationFile { + /// Schema version. + #[serde( + default = "default_version", + deserialize_with = "yaml::flexible_string" + )] + pub schema_version: String, + + /// One entry per calibrated item, by item id. + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + pub items: BTreeMap, +} + +impl CalibrationFile { + /// Loads the store, or an empty one when the file does not exist yet. + /// + /// Absence is not an error: a course that has not graded anything has no + /// statistics, and every command that reads them has to work anyway. + /// + /// # Arguments + /// + /// * `path` - the file, usually `analysis/calibration.yaml`. + /// + /// # Returns + /// + /// The store. + /// + /// # Errors + /// + /// Returns [`Error::Yaml`] when the file exists and does not parse. + pub fn load(path: &Path) -> Result { + if !path.is_file() { + return Ok(CalibrationFile::default()); + } + yaml::read(path) + } + + /// Writes the store. + /// + /// # Arguments + /// + /// * `path` - the destination. + /// + /// # Errors + /// + /// Returns [`Error::Io`] on a write failure. + pub fn save(&self, path: &Path) -> Result<()> { + yaml::write(path, self) + } + + /// The calibration recorded for one item. + /// + /// # Arguments + /// + /// * `item` - the item id. + /// + /// # Returns + /// + /// The record, or `None` when the item has never been calibrated. + pub fn get(&self, item: &str) -> Option<&Calibration> { + self.items.get(item) + } + + /// Checks the store against the bank it describes. + /// + /// The checks that matter for a file kept apart from what it refers to: a + /// record for an item that no longer exists, an option id the item does not + /// have, and — the one worth having — statistics measured against a stem + /// that has since been reworded, which since 2.0 means they describe a + /// different question wearing the same id. + /// + /// # Arguments + /// + /// * `catalog` - the loaded course. + /// + /// # Returns + /// + /// One message per problem, empty when the store agrees with the bank. + pub fn validate(&self, catalog: &crate::catalog::Catalog) -> Vec { + let mut issues = Vec::new(); + + for (id, calibration) in &self.items { + let Some(entry) = catalog.get(id) else { + issues.push(format!( + "calibration for `{id}`: no such item. Statistics outlive an item only if \ + it is retired, not deleted — a retired item keeps its id so its numbers \ + still mean something." + )); + continue; + }; + let item = &entry.item; + + for variant in &calibration.variants { + for option in variant.option_stats.keys() { + if item.option(option).is_none() { + issues.push(format!( + "calibration for `{id}`: variant `{}` has statistics for `{option}`, \ + which is not an option of this item", + short(&variant.variant) + )); + } + } + if !variant.key.is_empty() + && variant.variant != item.variant_digest(&variant.key, &variant.distractors) + { + issues.push(format!( + "calibration for `{id}`: variant `{}` was measured against an option set \ + that has since been reworded, so its numbers describe wording no \ + student now sees", + short(&variant.variant) + )); + } + } + for option in calibration.options.keys() { + if item.option(option).is_none() { + issues.push(format!( + "calibration for `{id}`: an option history names `{option}`, which is \ + not an option of this item" + )); + } + } + } + issues + } +} + +/// What one administration measured: `analysis/administrations/.yaml`. +/// +/// Written once, when the exam is analyzed, and never rewritten. It is the +/// audit trail under [`CalibrationFile`]: the pooled numbers say an item sits +/// at 0.63, and these say which exams that came from and what each one saw. +/// Keeping them also means the history survives losing `data/`, which is +/// ignored by git and rotates. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct MeasurementFile { + /// Schema version. + #[serde( + default = "default_version", + deserialize_with = "yaml::flexible_string" + )] + pub schema_version: String, + + /// What was administered, and how it was analyzed. + pub administration: MeasurementMeta, + + /// One entry per question, in the order it was printed. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub items: Vec, +} + +/// What an administration was, for a reader two years later. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct MeasurementMeta { + /// The administration id these numbers came from. + pub id: String, + /// The assessment that was administered. + pub assessment: String, + /// The term. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub term: Option, + /// The date it was given. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub date: Option, + /// The forms in play. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub forms: Vec, + /// How many examinees the numbers pool over. + /// + /// The one number to read before any of the others. A point-biserial on + /// twenty-seven students is a different kind of claim than one on three + /// hundred, and nothing below records how thin it is. + pub n_examinees: usize, + /// The item response model fitted, when one was. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub model: Option, + /// When the analysis was run. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub generated: Option, + /// The version of the tool that ran it, since the numbers depend on it. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub coursebank: Option, +} + +/// One question's statistics from one administration. +/// +/// Cohort aggregates only. There is deliberately no per-section or per-form +/// breakdown: those get small, and a small cell crossed with anything else is +/// how an aggregate stops being one. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Measurement { + /// The item id. + pub item: String, + /// The question number it was printed as. + pub number: u32, + /// The option set administered. See [`crate::item::Item::variant_digest`]. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub variant: Option, + /// The stem as administered. See [`crate::item::Item::stem_digest`]. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub stem_digest: Option, + /// Examinees who saw it. + pub n: usize, + /// Proportion correct. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub p_value: Option, + /// Corrected item-total point-biserial correlation. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub point_biserial: Option, + /// Upper-minus-lower-group discrimination index. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub discrimination_index: Option, + /// The option ids keyed correct, so a row in the options file says whether + /// it describes the answer or a distractor without a join. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub key: Vec, + /// Per-option behaviour, by option id. + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + pub option_stats: BTreeMap, + /// Fitted parameters, when the sample supported a fit. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub irt: Option, + /// Machine-detected problems with this question on this administration. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub flags: Vec, +} + +/// The columns of an items file, and the only ones accepted on read. +/// +/// An allowlist rather than a type-level guarantee. In YAML the structures +/// reject unknown keys, so a file carrying a student column could not be +/// loaded; CSV readers are tolerant of extra columns, so the same assurance has +/// to be an explicit check. This is it, and [`MeasurementFile::read_csv`] +/// refuses any header not named here. +pub const ITEM_COLUMNS: [&str; 16] = [ + "administration_id", + "assessment", + "term", + "date", + "forms", + "n_examinees", + "coursebank", + "generated", + "item", + "number", + "variant", + "stem_digest", + "n", + "p_value", + "point_biserial", + "discrimination_index", +]; + +/// The columns of an options file, and the only ones accepted on read. +pub const OPTION_COLUMNS: [&str; 9] = [ + "administration_id", + "item", + "number", + "option", + "keyed", + "selection_rate", + "point_biserial", + "upper_group_rate", + "lower_group_rate", +]; + +/// One row of an items file. +#[derive(Debug, Clone, Serialize, Deserialize)] +struct ItemRow { + administration_id: String, + assessment: String, + term: String, + date: String, + forms: String, + n_examinees: usize, + coursebank: String, + generated: String, + item: String, + number: u32, + variant: String, + stem_digest: String, + n: usize, + p_value: String, + point_biserial: String, + discrimination_index: String, +} + +/// One row of an options file. +#[derive(Debug, Clone, Serialize, Deserialize)] +struct OptionRow { + administration_id: String, + item: String, + number: u32, + option: String, + keyed: bool, + selection_rate: String, + point_biserial: String, + upper_group_rate: String, + lower_group_rate: String, +} + +impl MeasurementFile { + /// The two file names this administration writes, items first. + /// + /// # Arguments + /// + /// * `dir` - usually `analysis/administrations`. + /// + /// # Returns + /// + /// The items path and the options path. + pub fn paths(&self, dir: &Path) -> (PathBuf, PathBuf) { + let stem = crate::course::slugify(&self.administration.id); + ( + dir.join(format!("{stem}-items.csv")), + dir.join(format!("{stem}-options.csv")), + ) + } + + /// Writes the two files, refusing to overwrite either. + /// + /// An administration is a thing that happened once, so replacing its record + /// is a deliberate act: delete the files first if you mean to re-analyze. + /// + /// Two files rather than one because the data is two shapes — one row per + /// question, one row per question and option — and a single sparse table + /// serves neither. The administration's metadata repeats on every row, + /// which is what makes each file independently loadable and is the same + /// convention the response store already uses. + /// + /// # Arguments + /// + /// * `dir` - the directory to write into. + /// + /// # Returns + /// + /// The paths written. + /// + /// # Errors + /// + /// Returns [`Error::Usage`] when either file exists, and [`Error::Io`] or + /// [`Error::Csv`] on a write failure. + pub fn write_csv(&self, dir: &Path) -> Result> { + let (items_path, options_path) = self.paths(dir); + for path in [&items_path, &options_path] { + if path.exists() { + return Err(Error::usage(format!( + "{} already records this administration. It happened once, so replacing it \ + is a deliberate act: delete it first if you mean to re-analyze.", + path.display() + ))); + } + } + std::fs::create_dir_all(dir).map_err(|e| Error::io(dir, e))?; + + let meta = &self.administration; + let mut items = csv::Writer::from_path(&items_path).map_err(|e| Error::Csv { + path: items_path.clone(), + source: e, + })?; + let mut options = csv::Writer::from_path(&options_path).map_err(|e| Error::Csv { + path: options_path.clone(), + source: e, + })?; + + for measurement in &self.items { + items + .serialize(ItemRow { + administration_id: meta.id.clone(), + assessment: meta.assessment.clone(), + term: meta.term.clone().unwrap_or_default(), + date: meta.date.map(|d| d.to_string()).unwrap_or_default(), + forms: meta.forms.join(";"), + n_examinees: meta.n_examinees, + coursebank: meta.coursebank.clone().unwrap_or_default(), + generated: meta.generated.map(|d| d.to_string()).unwrap_or_default(), + item: measurement.item.clone(), + number: measurement.number, + variant: measurement.variant.clone().unwrap_or_default(), + stem_digest: measurement.stem_digest.clone().unwrap_or_default(), + n: measurement.n, + p_value: number(measurement.p_value), + point_biserial: number(measurement.point_biserial), + discrimination_index: number(measurement.discrimination_index), + }) + .map_err(|e| Error::Csv { + path: items_path.clone(), + source: e, + })?; + + for (option, stat) in &measurement.option_stats { + options + .serialize(OptionRow { + administration_id: meta.id.clone(), + item: measurement.item.clone(), + number: measurement.number, + option: option.clone(), + keyed: measurement.key.iter().any(|k| k == option), + selection_rate: number(stat.selection_rate), + point_biserial: number(stat.point_biserial), + upper_group_rate: number(stat.upper_group_rate), + lower_group_rate: number(stat.lower_group_rate), + }) + .map_err(|e| Error::Csv { + path: options_path.clone(), + source: e, + })?; + } + } + + items.flush().map_err(|e| Error::io(&items_path, e))?; + options.flush().map_err(|e| Error::io(&options_path, e))?; + Ok(vec![items_path, options_path]) + } + + /// Reads one administration back from its two files. + /// + /// # Arguments + /// + /// * `items_path` - the items file. The options file is found beside it. + /// + /// # Returns + /// + /// The administration, with its per-option statistics reattached. + /// + /// # Errors + /// + /// Returns [`Error::Csv`] on a parse failure and [`Error::Invalid`] when a + /// file carries a column that is not in [`ITEM_COLUMNS`] or + /// [`OPTION_COLUMNS`] — which is how a student column is caught. + pub fn read_csv(items_path: &Path) -> Result { + let options_path = PathBuf::from( + items_path + .to_string_lossy() + .replace("-items.csv", "-options.csv"), + ); + + let mut reader = open_csv(items_path, &ITEM_COLUMNS)?; + let mut meta = MeasurementMeta::default(); + let mut items: Vec = Vec::new(); + for row in reader.deserialize::() { + let row = row.map_err(|e| Error::Csv { + path: items_path.to_path_buf(), + source: e, + })?; + meta = MeasurementMeta { + id: row.administration_id.clone(), + assessment: row.assessment.clone(), + term: some(&row.term), + date: parse_date(&row.date), + forms: row + .forms + .split(';') + .filter(|f| !f.is_empty()) + .map(str::to_string) + .collect(), + n_examinees: row.n_examinees, + model: meta.model, + generated: parse_date(&row.generated), + coursebank: some(&row.coursebank), + }; + items.push(Measurement { + item: row.item, + number: row.number, + variant: some(&row.variant), + stem_digest: some(&row.stem_digest), + n: row.n, + p_value: parse(&row.p_value), + point_biserial: parse(&row.point_biserial), + discrimination_index: parse(&row.discrimination_index), + ..Measurement::default() + }); + } + + if options_path.is_file() { + let mut reader = open_csv(&options_path, &OPTION_COLUMNS)?; + for row in reader.deserialize::() { + let row = row.map_err(|e| Error::Csv { + path: options_path.clone(), + source: e, + })?; + let Some(target) = items.iter_mut().find(|i| i.number == row.number) else { + continue; + }; + if row.keyed && !target.key.contains(&row.option) { + target.key.push(row.option.clone()); + } + target.option_stats.insert( + row.option, + OptionStat { + selection_rate: parse(&row.selection_rate), + point_biserial: parse(&row.point_biserial), + upper_group_rate: parse(&row.upper_group_rate), + lower_group_rate: parse(&row.lower_group_rate), + }, + ); + } + } + + Ok(MeasurementFile { + schema_version: default_version(), + administration: meta, + items, + }) + } + + /// Reads every administration under a directory, oldest first. + /// + /// # Arguments + /// + /// * `dir` - usually `analysis/administrations`. + /// + /// # Returns + /// + /// The administrations, empty when the directory does not exist. + /// + /// # Errors + /// + /// Propagates read failures. + pub fn load_all(dir: &Path) -> Result> { + if !dir.is_dir() { + return Ok(Vec::new()); + } + let mut paths: Vec = std::fs::read_dir(dir) + .map_err(|e| Error::io(dir, e))? + .filter_map(|e| e.ok().map(|e| e.path())) + .filter(|p| p.to_string_lossy().ends_with("-items.csv")) + .collect(); + paths.sort(); + + let mut out = Vec::new(); + for path in &paths { + out.push(MeasurementFile::read_csv(path)?); + } + out.sort_by(|a, b| { + a.administration + .date + .cmp(&b.administration.date) + .then(a.administration.id.cmp(&b.administration.id)) + }); + Ok(out) + } +} + +/// Opens a CSV and refuses any column that is not on the allowlist. +fn open_csv(path: &Path, allowed: &[&str]) -> Result> { + let mut reader = csv::Reader::from_path(path).map_err(|e| Error::Csv { + path: path.to_path_buf(), + source: e, + })?; + let headers = reader + .headers() + .map_err(|e| Error::Csv { + path: path.to_path_buf(), + source: e, + })? + .clone(); + + let unexpected: Vec<&str> = headers.iter().filter(|h| !allowed.contains(h)).collect(); + if !unexpected.is_empty() { + return Err(Error::Invalid(vec![format!( + "{}: unexpected column(s) {}. These files are committed, so they hold cohort \ + aggregates and nothing else — anything per-student belongs in data/, which is \ + ignored.", + path.display(), + unexpected.join(", ") + )])); + } + Ok(reader) +} + +/// A float as a CSV cell, empty when absent. +fn number(value: Option) -> String { + value.map(|v| format!("{v}")).unwrap_or_default() +} + +/// A CSV cell as a float, absent when empty or unparseable. +fn parse(cell: &str) -> Option { + cell.trim().parse().ok() +} + +/// A CSV cell as a string, absent when empty. +fn some(cell: &str) -> Option { + (!cell.trim().is_empty()).then(|| cell.trim().to_string()) +} + +/// A CSV cell as a date, absent when empty or unparseable. +fn parse_date(cell: &str) -> Option { + some(cell).and_then(|c| c.parse().ok()) +} + +/// The schema version new files are written with. +fn default_version() -> String { + SCHEMA_VERSION.to_string() +} + +/// A digest shortened for a message. +fn short(digest: &str) -> String { + digest.chars().take(8).collect() +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::item::VariantCalibration; + + fn tmp(tag: &str) -> std::path::PathBuf { + let p = std::env::temp_dir().join(format!("coursebank-cal-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&p); + std::fs::create_dir_all(&p).unwrap(); + p + } + + #[test] + fn an_absent_store_is_empty_rather_than_an_error() { + let dir = tmp("absent"); + let store = CalibrationFile::load(&dir.join(CALIBRATION_FILE)).unwrap(); + assert!(store.items.is_empty()); + assert!(store.get("q-x").is_none()); + } + + #[test] + fn the_store_round_trips() { + let dir = tmp("round"); + let path = dir.join(CALIBRATION_FILE); + + let mut store = CalibrationFile::default(); + store.items.insert( + "q-x".to_string(), + Calibration { + n_examinees: Some(27), + p_value: Some(0.63), + variants: vec![VariantCalibration { + variant: "4c81fa".into(), + n_examinees: Some(27), + ..VariantCalibration::default() + }], + ..Calibration::default() + }, + ); + store.save(&path).unwrap(); + + let back = CalibrationFile::load(&path).unwrap(); + assert_eq!(back.get("q-x").unwrap().p_value, Some(0.63)); + assert_eq!(back.get("q-x").unwrap().variants.len(), 1); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn an_administration_round_trips_through_two_csvs() { + let dir = tmp("csv"); + let file = MeasurementFile { + schema_version: default_version(), + administration: MeasurementMeta { + id: "e1-2026f".into(), + assessment: "e1".into(), + term: Some("2026f".into()), + date: Some(Date::new(2026, 9, 15).unwrap()), + forms: vec!["A".into(), "B".into()], + n_examinees: 27, + model: None, + generated: Some(Date::new(2026, 9, 27).unwrap()), + coursebank: Some("0.0.0".into()), + }, + items: vec![Measurement { + item: "q-fastq-quality-length-match".into(), + number: 1, + variant: Some("237d62f9af222f78".into()), + stem_digest: Some("8b22e0".into()), + n: 27, + p_value: Some(0.5926), + point_biserial: Some(0.31), + discrimination_index: None, + key: vec!["o-fourth-line".into()], + option_stats: [ + ( + "o-fourth-line".to_string(), + OptionStat { + selection_rate: Some(0.5926), + point_biserial: Some(0.31), + upper_group_rate: None, + lower_group_rate: None, + }, + ), + ( + "o-third-line".to_string(), + OptionStat { + selection_rate: Some(0.1852), + point_biserial: Some(-0.18), + upper_group_rate: None, + lower_group_rate: None, + }, + ), + ] + .into_iter() + .collect(), + irt: None, + flags: Vec::new(), + }], + }; + + let written = file.write_csv(&dir).unwrap(); + assert_eq!(written.len(), 2, "one table per shape"); + assert!(written[0].ends_with("e1-2026f-items.csv")); + assert!(written[1].ends_with("e1-2026f-options.csv")); + + // One line per question, loadable on its own: the administration's + // metadata repeats on the row, as the response store already does. + let items = std::fs::read_to_string(&written[0]).unwrap(); + assert!( + items.starts_with("administration_id,assessment,term,date,forms"), + "{items}" + ); + assert!( + items.contains("e1-2026f,e1,2026f,2026-09-15,A;B,27"), + "{items}" + ); + + let options = std::fs::read_to_string(&written[1]).unwrap(); + assert!(options.contains("o-fourth-line,true"), "{options}"); + assert!(options.contains("o-third-line,false"), "{options}"); + + let back = MeasurementFile::read_csv(&written[0]).unwrap(); + assert_eq!(back.administration.id, "e1-2026f"); + assert_eq!(back.administration.n_examinees, 27); + assert_eq!(back.administration.forms, vec!["A", "B"]); + assert_eq!(back.items.len(), 1); + assert_eq!(back.items[0].p_value, Some(0.5926)); + assert_eq!(back.items[0].key, vec!["o-fourth-line"]); + assert_eq!(back.items[0].option_stats.len(), 2); + assert_eq!(back.items[0].discrimination_index, None); + + assert_eq!(MeasurementFile::load_all(&dir).unwrap().len(), 1); + + // It happened once, so the record is not replaced by accident. + let err = file.write_csv(&dir).unwrap_err().to_string(); + assert!(err.contains("deliberate"), "{err}"); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn a_student_column_is_refused_on_read() { + let dir = tmp("identifiers"); + let path = dir.join("e1-2026f-items.csv"); + std::fs::write( + &path, + "administration_id,assessment,item,number,n,student_key\n e1-2026f,e1,q-x,1,27,abc123\n", + ) + .unwrap(); + + // In YAML this fell out of the type, which rejects unknown keys. A CSV + // reader tolerates extra columns, so the same assurance has to be an + // explicit allowlist — and this is the test that it is one. + let err = MeasurementFile::read_csv(&path).unwrap_err().to_string(); + assert!(err.contains("student_key"), "{err}"); + assert!(err.contains("cohort aggregates"), "{err}"); + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/src/model/catalog.rs b/src/model/catalog.rs index f4fb175..3e1a15e 100644 --- a/src/model/catalog.rs +++ b/src/model/catalog.rs @@ -75,6 +75,12 @@ pub struct Catalog { pub entries: Vec, /// Bank metadata by bank id. pub banks: BTreeMap, + /// The statistics, loaded from `analysis/` rather than from the banks. + /// + /// Each entry's [`crate::item::Item::calibration`] is filled from this at + /// load, so everything downstream reads one item and does not have to know + /// that the numbers and the wording come from different files. + pub calibration: crate::calibration::CalibrationFile, /// Map from global id to index into `entries`. index: BTreeMap, } @@ -104,7 +110,10 @@ impl Catalog { entries: Vec::new(), banks: BTreeMap::new(), index: BTreeMap::new(), + calibration: crate::calibration::CalibrationFile::default(), }; + catalog.calibration = + crate::calibration::CalibrationFile::load(&catalog.layout.calibration_file())?; let mut problems = Vec::new(); let mut files = yaml::list_yaml(&catalog.layout.banks())?; @@ -145,6 +154,15 @@ impl Catalog { if !problems.is_empty() { return Err(Error::Invalid(problems)); } + // Statistics are attached here rather than parsed from the bank, which + // is why an item can be read as one thing while its wording and its + // evidence live in files with different review cycles. + for entry in &mut catalog.entries { + if let Some(calibration) = catalog.calibration.items.get(&entry.uid) { + entry.item.calibration = Some(calibration.clone()); + } + } + Ok(catalog) } diff --git a/src/model/course.rs b/src/model/course.rs index 3d93428..d9746a7 100644 --- a/src/model/course.rs +++ b/src/model/course.rs @@ -800,15 +800,25 @@ pub struct Objective { /// The lectures that develop it. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub lectures: Vec, - /// Position in teaching order, low first. + /// Position in teaching order, low first. Derived; authoring it is + /// deprecated. /// - /// The registry is a map, so declaration order is lost on load, and sorting + /// The registry is a map, so declaration order is lost on load and sorting /// by id would put `lo-enthalpy` before `lo-first-law` when the second is a - /// prerequisite of the first. Anything that prints objectives in the order - /// you teach them, a lecture page above all, needs this. Objectives without - /// it sort last, by id. + /// prerequisite of the first. Something has to supply the order. + /// + /// Since 2.0 that something is [`Lecture::teaches`], which is a sequence: + /// the position of an objective in the list of what a lecture covers, and + /// the position of the lecture in the course, together say when it is + /// taught. [`fragment::assemble`] fills this in from those two, so a course + /// that used to carry thirty-nine hand-kept integers now carries none, and + /// inserting an objective is a one-line edit rather than a renumber. + /// + /// An authored value still wins, so a 1.0 course loads unchanged. + /// `coursebank migrate order` removes them. #[serde(default, skip_serializing_if = "Option::is_none")] pub order: Option, + /// The highest level you intend to assess this objective at. Assembling an /// item above the ceiling is a warning: either the item overreaches or the /// ceiling needs raising. @@ -865,8 +875,20 @@ pub struct Target { /// /// Ordered within its objective rather than across the course, so two /// targets under different objectives never compete for a position and - /// inserting one renumbers nothing outside its own group. - #[serde(default, skip_serializing_if = "Option::is_none")] + /// Retained only so a pre-2.0 course still loads. Ignored. + /// + /// Targets do not have an order. An objective's targets are a set of + /// question templates, not steps in a sequence: they are not taught in + /// order, an exam samples from them rather than working through them, and + /// the study workflow reads the list as a checklist and counts what it can + /// do cold. A position would assert a sequence that does not exist. + /// + /// Where a list has to be printed, [`CourseFile::targets`] orders it by + /// ceiling and then by id. Where one target genuinely depends on another, + /// that is [`Target::prerequisites`], which says so directly. + /// + /// `coursebank migrate order` removes it. + #[serde(default, skip_serializing)] pub order: Option, /// The highest level you intend to assess this target at. Omit it to inherit /// the objective's. @@ -1183,6 +1205,33 @@ impl CourseFile { )); } } + // A manuscript with no journal is a citation nobody can print. The + // fields exist; a note that carries them instead is data the reading + // list cannot link and the bibliography exporters cannot use. + if matches!( + reference.kind, + ReferenceKind::Article | ReferenceKind::Preprint + ) && reference.container.is_none() + { + issues.push(format!( + "reference `{key}`: an {} needs a `container` — the journal, preprint \ + server, or proceedings it appeared in. Run `coursebank migrate references` \ + if it is sitting in the `note`.", + match reference.kind { + ReferenceKind::Preprint => "preprint", + _ => "article", + } + )); + } + if let Some(note) = &reference.note { + if crate::citation::looks_like_a_citation(note) { + issues.push(format!( + "reference `{key}`: the note still carries a citation. Volume, pages, \ + and DOI have their own fields, and a DOI in a note is a link nobody \ + can follow. `coursebank migrate references` takes it apart." + )); + } + } if let Some(pmid) = &reference.pmid { if !pmid.trim().chars().all(|c| c.is_ascii_digit()) { issues.push(format!( @@ -1646,9 +1695,20 @@ impl CourseFile { .filter(|(_, target)| target.objective == objective) .map(|(id, _)| id) .collect(); + // By ceiling, then by id. A target is a question template rather than a + // step in a sequence — an objective's targets are not taught in an + // order, and an exam samples from them — so there is no teaching order + // to print. What there is is depth, and grouping by it puts the + // checklist in the order the study methods apply: recall for a Level 1 + // target, explanation for Level 2, variations and written solutions + // above that. Ties break by id so the list is stable. ids.sort_by_key(|id| { - let target = &self.learning_targets[*id]; - (target.order.unwrap_or(u32::MAX), (*id).clone()) + ( + self.effective_level_ceiling(id) + .map(|l| l.code()) + .unwrap_or(0), + (*id).clone(), + ) }); ids.into_iter().map(String::as_str).collect() } diff --git a/src/model/course/fragment.rs b/src/model/course/fragment.rs index 7a88885..83a1e5d 100644 --- a/src/model/course/fragment.rs +++ b/src/model/course/fragment.rs @@ -535,6 +535,27 @@ impl Merge { } } + // Teaching order, from the two sequences that already declare it: the + // lectures in course order, and each lecture's `teaches` list. This is + // what lets an objective stop carrying a hand-kept integer. + let mut position = 0u32; + let ordered: Vec = self.lectures.keys().cloned().collect(); + for lecture_id in ordered { + let teaches = self + .lectures + .get(&lecture_id) + .map(|l| l.teaches.clone()) + .unwrap_or_default(); + for objective_id in teaches { + position += 1; + if let Some(objective) = self.objectives.get_mut(&objective_id) { + if objective.order.is_none() { + objective.order = Some(position); + } + } + } + } + // A target with no lecture of its own is taught wherever its objective // is. Collected first: the read of `objectives` and the write to // `targets` cannot overlap in one pass. @@ -800,6 +821,93 @@ units: assert!(course.origin("nothing-like-this").is_none()); } + #[test] + fn teaching_order_is_derived_from_the_two_sequences_that_declare_it() { + let root = tmp("order"); + write(&root, "course.yaml", ROOT); + write( + &root, + "lectures/l-1-2.yaml", + "lectures:\n L1.2:\n title: One\n teaches: [lo-second, lo-first]\n", + ); + write( + &root, + "lectures/l-1-3.yaml", + "lectures:\n L1.3:\n title: Two\n teaches: [lo-third]\n", + ); + write( + &root, + "objectives/lo-first.yaml", + r#"learning_objectives: + lo-first: + text: A. + lo-second: + text: B. + lo-third: + text: C. +"#, + ); + + let course = assemble(&root).unwrap(); + // Position in `teaches`, not id order: the lecture lists `lo-second` + // first and that is what teaching it first means. + assert_eq!(course.learning_objectives["lo-second"].order, Some(1)); + assert_eq!(course.learning_objectives["lo-first"].order, Some(2)); + assert_eq!(course.learning_objectives["lo-third"].order, Some(3)); + assert_eq!( + course.lecture_objectives("L1.2"), + vec!["lo-second", "lo-first"] + ); + } + + #[test] + fn targets_print_by_ceiling_rather_than_in_a_sequence() { + let root = tmp("target-order"); + write(&root, "course.yaml", ROOT); + write( + &root, + "lectures/l-1-2.yaml", + "lectures:\n L1.2:\n title: One\n teaches: [lo-x]\n", + ); + write( + &root, + "objectives/lo-x.yaml", + r#"learning_objectives: + lo-x: + text: A. + level_ceiling: 3 +learning_targets: + t-predict: + text: Predict the effect. + objective: lo-x + level_ceiling: 3 + t-define: + text: Define the term. + objective: lo-x + level_ceiling: 1 + t-explain: + text: Explain the mechanism. + objective: lo-x + level_ceiling: 2 +"#, + ); + + let course = assemble(&root).unwrap(); + assert!(course.validate().is_empty(), "{:?}", course.validate()); + + // Shallowest first, which is the order the study methods apply in: + // recall, then explanation, then variations. Not id order, which would + // put `t-define` after `t-predict` for no reason at all. + assert_eq!( + course.targets("lo-x"), + vec!["t-define", "t-explain", "t-predict"] + ); + + // A target with no ceiling of its own inherits the objective's, so it + // sorts where that puts it. + assert_eq!(course.learning_targets["t-define"].order, None); + } + #[test] fn teaching_the_same_objective_from_two_lectures_unions() { let root = tmp("union"); diff --git a/src/model/item.rs b/src/model/item.rs index 34d2859..a5e8ca8 100644 --- a/src/model/item.rs +++ b/src/model/item.rs @@ -157,7 +157,14 @@ pub struct Item { pub design: Option, /// What the evidence says, accumulated across administrations. - #[serde(default, skip_serializing_if = "Option::is_none")] + /// What the statistics say, filled in from `analysis/` at load. + /// + /// Read from the store and never written back: `skip_serializing` means a + /// bank file cannot acquire a `calibration:` block by being round-tripped + /// through this type. A bank is a reviewed artifact whose diff should be a + /// change of intent, and every grading run would otherwise produce a diff + /// on it. See [`crate::calibration`]. + #[serde(default, skip_serializing)] pub calibration: Option, /// The last review decision recorded for this item. diff --git a/src/model/layout.rs b/src/model/layout.rs index 16772bc..632b097 100644 --- a/src/model/layout.rs +++ b/src/model/layout.rs @@ -64,6 +64,24 @@ impl Layout { self.root.join("banks") } + /// Directory holding the statistics, which are kept out of the banks. + /// + /// Committed, unlike [`Layout::data`]: everything under here is a cohort + /// aggregate with no student in it. See [`crate::calibration`]. + pub fn analysis(&self) -> PathBuf { + self.root.join("analysis") + } + + /// The pooled per-item calibration store. + pub fn calibration_file(&self) -> PathBuf { + self.analysis().join(crate::calibration::CALIBRATION_FILE) + } + + /// Directory holding one immutable record per administration. + pub fn measurements(&self) -> PathBuf { + self.analysis().join("administrations") + } + /// Directory holding assessment records. pub fn assessments(&self) -> PathBuf { self.root.join("assessments") @@ -117,6 +135,8 @@ impl Layout { pub fn create_all(&self) -> Result<()> { for dir in [ self.root.clone(), + self.analysis(), + self.measurements(), self.lectures(), self.objectives(), self.banks(), diff --git a/src/model/seal.rs b/src/model/seal.rs index d530368..a6d760f 100644 --- a/src/model/seal.rs +++ b/src/model/seal.rs @@ -124,6 +124,26 @@ pub struct SealMeta { /// from the file's own contents; [`SealFile::verify`] rebuilds a seal from the /// live course and compares this against it. pub digest: String, + + /// The digests this file had before it was rewritten, oldest first. + /// + /// A seal is meant to be tamper-evident, which puts a migration that + /// renames an id inside it in an awkward position: the id is part of + /// [`SealFile::digest_input`], so rewriting the file invalidates the digest + /// that vouched for it, and recomputing it quietly would produce a record + /// that looks untouched and is not. + /// + /// So a migration does both — recomputes the digest and leaves the old one + /// here. What the seal then says is the honest thing: these are the + /// contents, this is what they hash to, and here is what the file hashed to + /// before each rewrite. Anyone holding an earlier copy, a backup or a git + /// revision, can check it against the right entry. + /// + /// Deliberately outside [`SealFile::digest_input`]: a field that recorded + /// past digests and was itself covered by the current one could not be + /// appended to without invalidating what it describes. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub superseded_digests: Vec, } /// What wrote a seal. @@ -458,6 +478,7 @@ pub fn build(catalog: &Catalog, record: &AssessmentFile, opts: &Options) -> Resu content: opts.content, dropped, digest: String::new(), + superseded_digests: Vec::new(), }, items, forms: sealed_forms, @@ -787,6 +808,39 @@ impl SealFile { buf } + /// Renames the items this seal froze, keeping the digest honest. + /// + /// The only sanctioned way to rewrite a seal. It recomputes the digest, + /// because the ids are part of what the digest covers, and records the + /// previous one in [`SealMeta::superseded_digests`], because a recomputed + /// digest with no trace of the recompute is a record that claims never to + /// have been touched. + /// + /// # Arguments + /// + /// * `rename` - a map from old item id to new. + /// + /// # Returns + /// + /// How many placements were renamed. Zero leaves the file alone, digest + /// included. + pub fn rename_items(&mut self, rename: &BTreeMap) -> usize { + let mut renamed = 0; + for item in &mut self.items { + if let Some(new) = rename.get(&item.item) { + item.item = new.clone(); + renamed += 1; + } + } + if renamed == 0 { + return 0; + } + let previous = std::mem::take(&mut self.seal.digest); + self.seal.superseded_digests.push(previous); + self.seal.digest = self.recompute_digest(); + renamed + } + /// Recomputes this file's digest from its own contents. /// /// # Returns @@ -1195,6 +1249,7 @@ mod tests { content: true, dropped: Vec::new(), digest: String::new(), + superseded_digests: Vec::new(), }, items: vec![SealedItem { number: 1, @@ -1343,6 +1398,31 @@ mod tests { assert_eq!(short("sha256:0123456789abcdef"), "sha256:01234567"); } + #[test] + fn renaming_an_item_recomputes_the_digest_and_says_it_did() { + let mut seal = sample(); + seal.seal.digest = seal.recompute_digest(); + let original = seal.seal.digest.clone(); + assert!(seal.check_self().is_empty()); + + let mut rename = BTreeMap::new(); + rename.insert(seal.items[0].item.clone(), "q-renamed".to_string()); + assert_eq!(seal.rename_items(&rename), 1); + + // The digest covers the ids, so it has to move — and the file has to + // admit that it moved rather than looking untouched. + assert_eq!(seal.items[0].item, "q-renamed"); + assert_ne!(seal.seal.digest, original); + assert_eq!(seal.seal.superseded_digests, vec![original]); + assert!(seal.check_self().is_empty(), "{:?}", seal.check_self()); + + // A rename that matches nothing leaves the file entirely alone. + let steady = seal.seal.digest.clone(); + assert_eq!(seal.rename_items(&BTreeMap::new()), 0); + assert_eq!(seal.seal.digest, steady); + assert_eq!(seal.seal.superseded_digests.len(), 1); + } + #[test] fn round_trips_through_yaml() { let file = sample(); diff --git a/src/util.rs b/src/util.rs index 83a30bb..2bdca5c 100644 --- a/src/util.rs +++ b/src/util.rs @@ -9,6 +9,7 @@ //! replaces a dependency that would otherwise need to keep working for as long as //! a course repository needs to stay readable. +pub mod citation; pub mod date; pub mod hash; pub mod markup; diff --git a/src/util/citation.rs b/src/util/citation.rs new file mode 100644 index 0000000..c4f5af2 --- /dev/null +++ b/src/util/citation.rs @@ -0,0 +1,330 @@ +// SPDX-License-Identifier: Prosperity-3.0.0 +// Copyright Scientific Computing Studio +// Source: https://git.scient.ing/education/coursebank + +//! Pulling a citation apart when it was written as prose. +//! +//! A bibliography assembled by hand tends to collect entries like +//! +//! ```text +//! note: 'Nucleic Acids Res 25:3389-3402. doi:10.1093/nar/25.17.3389' +//! ``` +//! +//! which is a complete citation in a field that means "anything else worth +//! saying". Nothing can use it: a reading list cannot link the DOI, an export to +//! Hayagriva or BibTeX has no journal to put in `parent` or `journal`, and the +//! `container`, `volume`, `pages`, and `doi` fields sit empty beside it. +//! +//! [`parse`] takes such a note apart. What it cannot account for it leaves in +//! the note, which is the important half of the contract: a note reading +//! `'Bioinformatics 18:440-445. Origin of spaced seeds.'` yields the journal, +//! the volume, the pages, and a note that still says where spaced seeds came +//! from. Nothing is discarded and nothing is invented — an issue number that was +//! never written down stays absent, even when a publisher's DOI happens to +//! encode one. + +/// The parts of a citation recovered from a note. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct Parsed { + /// The journal, proceedings, or book the work appeared in. + pub container: Option, + /// The volume. + pub volume: Option, + /// The page range, as `first-last`. + pub pages: Option, + /// The DOI, bare. + pub doi: Option, + /// What the note still says after the citation is removed. + pub note: Option, +} + +/// Takes a citation apart, leaving the rest of the note alone. +/// +/// # Arguments +/// +/// * `note` - the note as written. +/// * `year` - the record's year, which is how a trailing year is recognized as +/// part of a conference name rather than part of the title of the venue. +/// +/// # Returns +/// +/// The parts found. Every field is independently optional: a note that carries +/// only a DOI yields only a DOI. +pub fn parse(note: &str, year: Option) -> Parsed { + let mut out = Parsed::default(); + let mut rest = note.trim().to_string(); + + if let Some((container, volume, pages, tail)) = citation(&rest) { + out.container = Some(container); + out.volume = Some(volume); + out.pages = Some(pages); + rest = tail; + } + + if let Some((doi, tail)) = doi(&rest) { + out.doi = Some(doi); + rest = tail; + } + + // A venue with no volume or pages — a conference, usually — is named by the + // clause that ends in the year the work was published. + if out.container.is_none() { + if let Some(y) = year { + if let Some((container, tail)) = venue(&rest, y) { + out.container = Some(container); + rest = tail; + } + } + } + + // The year belongs to the record, not to the name of the venue. + if let (Some(container), Some(y)) = (&out.container, year) { + let suffix = format!(" {y}"); + if let Some(trimmed) = container.strip_suffix(&suffix) { + out.container = Some(trimmed.trim_end().to_string()); + } + } + + let rest = rest.trim().trim_start_matches('.').trim().to_string(); + out.note = (!rest.is_empty()).then_some(rest); + out +} + +/// Finds `Journal 25:3389-3402` or `Journal 48, 443-453` at the start. +/// +/// The volume is the first digit run that follows a space and is followed by a +/// separator and a page range. Requiring the whole shape is what keeps a year in +/// a conference name (`Proc. FOCS 2000.`) from being read as a volume. +/// +/// # Returns +/// +/// The container, volume, page range, and whatever followed. +fn citation(text: &str) -> Option<(String, String, String, String)> { + let bytes = text.as_bytes(); + let mut at = 0; + + while at < bytes.len() { + // A volume follows a space, so that a digit inside a name is not one. + if !(bytes[at].is_ascii_digit() && at > 0 && bytes[at - 1] == b' ') { + at += 1; + continue; + } + let volume_start = at; + let volume_end = digits(bytes, volume_start); + let mut cursor = spaces(bytes, volume_end); + + // The separator between volume and pages is a colon or a comma. + if cursor < bytes.len() && (bytes[cursor] == b':' || bytes[cursor] == b',') { + cursor = spaces(bytes, cursor + 1); + let first_start = cursor; + let first_end = digits(bytes, first_start); + if first_end > first_start { + let dash = text[first_end..] + .strip_prefix('-') + .or_else(|| text[first_end..].strip_prefix('\u{2013}')); + if let Some(after_dash) = dash { + let last_offset = text.len() - after_dash.len(); + let last_end = digits(bytes, last_offset); + if last_end > last_offset { + let container = text[..volume_start].trim_end_matches([' ', ',']); + if !container.is_empty() { + return Some(( + container.to_string(), + text[volume_start..volume_end].to_string(), + format!( + "{}-{}", + &text[first_start..first_end], + &text[last_offset..last_end] + ), + text[last_end..].to_string(), + )); + } + } + } + } + } + at = volume_end; + } + None +} + +/// Finds a `doi:10.…` anywhere in the text. +/// +/// # Returns +/// +/// The DOI and the text with it removed. +fn doi(text: &str) -> Option<(String, String)> { + let lower = text.to_ascii_lowercase(); + let at = lower.find("doi:")?; + let after = text[at + 4..].trim_start(); + let offset = text.len() - after.len(); + let end = after + .find(char::is_whitespace) + .map(|n| offset + n) + .unwrap_or(text.len()); + + let doi = text[offset..end].trim_end_matches('.'); + if !doi.starts_with("10.") { + return None; + } + let mut remainder = String::from(text[..at].trim_end()); + let tail = text[end..].trim(); + if !tail.is_empty() { + if !remainder.is_empty() { + remainder.push(' '); + } + remainder.push_str(tail); + } + Some((doi.to_string(), remainder)) +} + +/// Finds a leading clause ending in the publication year: `Proc. FOCS 2000.` +/// +/// # Returns +/// +/// The clause without its trailing period, and whatever followed. +fn venue(text: &str, year: u32) -> Option<(String, String)> { + let needle = format!("{year}."); + let at = text.find(&needle)?; + let clause = text[..at + needle.len() - 1].trim(); + if clause.is_empty() { + return None; + } + Some(( + clause.to_string(), + text[at + needle.len()..].trim().to_string(), + )) +} + +/// The end of a run of ASCII digits starting at `from`. +fn digits(bytes: &[u8], from: usize) -> usize { + let mut at = from; + while at < bytes.len() && bytes[at].is_ascii_digit() { + at += 1; + } + at +} + +/// The end of a run of spaces starting at `from`. +fn spaces(bytes: &[u8], from: usize) -> usize { + let mut at = from; + while at < bytes.len() && bytes[at] == b' ' { + at += 1; + } + at +} + +/// Whether a note still looks like it is carrying a citation. +/// +/// Used by validation to say so, rather than leaving a note that a reading list +/// cannot link and an export cannot use. +/// +/// # Arguments +/// +/// * `note` - the note as written. +/// +/// # Returns +/// +/// `true` when a volume and page range, or a DOI, can be found in it. +pub fn looks_like_a_citation(note: &str) -> bool { + citation(note.trim()).is_some() || doi(note.trim()).is_some() +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Every article note in a real course bibliography, which is where the + /// shapes below come from. Two separator styles, DOIs in three positions, + /// a conference with no volume, and prose that has to survive. + #[test] + fn a_journal_citation_comes_apart() { + let p = parse( + "Nucleic Acids Res 25:3389-3402. doi:10.1093/nar/25.17.3389", + Some(1997), + ); + assert_eq!(p.container.as_deref(), Some("Nucleic Acids Res")); + assert_eq!(p.volume.as_deref(), Some("25")); + assert_eq!(p.pages.as_deref(), Some("3389-3402")); + assert_eq!(p.doi.as_deref(), Some("10.1093/nar/25.17.3389")); + assert_eq!(p.note, None); + // The DOI encodes volume 25, issue 17. Nothing infers the issue from + // it: a field nobody wrote down stays empty. + } + + #[test] + fn an_abbreviation_keeps_its_final_period() { + let p = parse("J. Mol. Biol. 48, 443-453.", Some(1970)); + assert_eq!(p.container.as_deref(), Some("J. Mol. Biol.")); + assert_eq!(p.volume.as_deref(), Some("48")); + assert_eq!(p.pages.as_deref(), Some("443-453")); + assert_eq!(p.note, None); + } + + #[test] + fn prose_after_a_citation_stays_in_the_note() { + let p = parse( + "Bioinformatics 18:440-445. Origin of spaced seeds.", + Some(2002), + ); + assert_eq!(p.container.as_deref(), Some("Bioinformatics")); + assert_eq!(p.note.as_deref(), Some("Origin of spaced seeds.")); + + // A caveat the author wrote is the last thing to throw away. + let p = parse("J Mol Biol 215:403-410. Verify before use.", Some(1990)); + assert_eq!(p.note.as_deref(), Some("Verify before use.")); + + let p = parse( + "Bioinformatics 25:2078-2079. doi:10.1093/bioinformatics/btp352. Author list is the \ + core set plus the 1000 Genomes Data Processing Subgroup; verify.", + Some(2009), + ); + assert_eq!(p.doi.as_deref(), Some("10.1093/bioinformatics/btp352")); + assert_eq!( + p.note.as_deref(), + Some( + "Author list is the core set plus the 1000 Genomes Data Processing Subgroup; \ + verify." + ) + ); + } + + #[test] + fn a_conference_has_a_year_where_a_volume_would_be() { + let p = parse( + "Proc. FOCS 2000. doi:10.1109/SFCS.2000.892127. The FM-index. Theory background.", + Some(2000), + ); + assert_eq!(p.container.as_deref(), Some("Proc. FOCS")); + // No volume and no pages were written, so none are invented — and the + // 2000 in the DOI is not mistaken for either. + assert_eq!(p.volume, None); + assert_eq!(p.pages, None); + assert_eq!(p.doi.as_deref(), Some("10.1109/SFCS.2000.892127")); + assert_eq!(p.note.as_deref(), Some("The FM-index. Theory background.")); + } + + #[test] + fn a_note_with_nothing_to_find_is_left_whole() { + let p = parse("Origin of the MAPQ score.", Some(2008)); + assert_eq!(p.container, None); + assert_eq!(p.note.as_deref(), Some("Origin of the MAPQ score.")); + } + + #[test] + fn a_multi_word_journal_is_not_cut_at_a_number() { + let p = parse("Advances in Mathematics 20, 367-387.", Some(1976)); + assert_eq!(p.container.as_deref(), Some("Advances in Mathematics")); + assert_eq!(p.volume.as_deref(), Some("20")); + } + + #[test] + fn what_validation_looks_for() { + assert!(looks_like_a_citation("Nat Methods 12:59-60.")); + assert!(looks_like_a_citation("doi:10.1038/nmeth.3176")); + assert!(!looks_like_a_citation("Origin of minimizers.")); + assert!(!looks_like_a_citation("Verify before use.")); + // A page range with no volume is not a citation shape. + assert!(!looks_like_a_citation("see pages 12-14")); + } +}