diff --git a/src/authoring/select.rs b/src/authoring/select.rs index cd4c983..a81fb9e 100644 --- a/src/authoring/select.rs +++ b/src/authoring/select.rs @@ -479,6 +479,7 @@ pub fn to_record( credit_overrides: BTreeMap::new(), dropped: false, dropped_as: None, + dropped_before_printing: false, }); } diff --git a/src/data/decode.rs b/src/data/decode.rs index 16534c1..cc9b209 100644 --- a/src/data/decode.rs +++ b/src/data/decode.rs @@ -81,6 +81,11 @@ pub enum Numbering { Recorded, } +/// The form marker put on a row that could not be translated, so it can be +/// removed after the borrow on `set.rows` ends. No real form id can collide +/// with it: form ids come from the record and are short labels like `A`. +const UNMAPPED: &str = "\u{1f}unmapped"; + /// One question's mapping on one form. #[derive(Debug, Clone)] pub struct QuestionMap { @@ -354,6 +359,11 @@ impl FormDecoder { self.by_position.len() } + /// Every recorded question number this form carries. + pub fn numbers(&self) -> impl Iterator + '_ { + self.by_position.values().map(|q| q.number) + } + /// Whether the form prints nothing, which means the record is empty. pub fn is_empty(&self) -> bool { self.by_position.is_empty() @@ -572,9 +582,17 @@ pub fn form_warning(claimed: &str, fits: &[FormFit]) -> Option { pub fn apply(set: &mut ResponseSet, decoder: &FormDecoder, numbering: Numbering) -> Vec { let mut warnings = Vec::new(); let mut unmapped_positions: BTreeSet = BTreeSet::new(); + let mut colliding_positions: BTreeSet = BTreeSet::new(); let mut unmapped_letters: BTreeSet = BTreeSet::new(); let mut translated = 0usize; + // Numbers this form really uses. An untranslated row whose raw number is one + // of these would silently masquerade as that question, and two rows would + // then share a number: one the student's answer to it, one an answer to + // something else entirely. Nothing downstream can tell them apart, so the + // collision has to be caught here. + let recorded: BTreeSet = decoder.numbers().collect(); + for row in &mut set.rows { let belongs = row .form @@ -587,6 +605,13 @@ pub fn apply(set: &mut ResponseSet, decoder: &FormDecoder, numbering: Numbering) let Some(map) = decoder.lookup(row.item_number, numbering) else { unmapped_positions.insert(row.item_number); + if recorded.contains(&row.item_number) { + colliding_positions.insert(row.item_number); + // Marked so the row can be discarded below. Attributing it to + // the question that legitimately holds this number would corrupt + // that question's statistics. + row.form = Some(UNMAPPED.to_string()); + } continue; }; @@ -615,9 +640,28 @@ pub fn apply(set: &mut ResponseSet, decoder: &FormDecoder, numbering: Numbering) if !unmapped_positions.is_empty() { let list: Vec = unmapped_positions.iter().map(|n| n.to_string()).collect(); warnings.push(format!( - "form {}: question(s) {} are in the export but not on this form; they were left \ - untranslated", + "form {}: question(s) {} are in the export but not on this form ({} printed). The \ + export may have been taken before a question was dropped, or from a different \ + form.", decoder.form, + list.join(", "), + decoder.len() + )); + } + if !colliding_positions.is_empty() { + let list: Vec = colliding_positions.iter().map(|n| n.to_string()).collect(); + let discarded = set.rows.len(); + set.rows + .retain(|row| row.form.as_deref() != Some(UNMAPPED)); + warnings.push(format!( + "form {}: {} response(s) at position(s) {} could not be translated, and their raw \ + numbers are numbers this form does use. Keeping them would have given those \ + questions two different answers each, so they were discarded. This is the shape of \ + an export made before a question was dropped: re-export the responses from the \ + administration you sealed, or re-run with --recorded-numbers if the export already \ + carries recorded numbers.", + decoder.form, + discarded - set.rows.len(), list.join(", ") )); } diff --git a/src/export/practice.rs b/src/export/practice.rs index 47f97c0..5ce8852 100644 --- a/src/export/practice.rs +++ b/src/export/practice.rs @@ -223,7 +223,7 @@ pub fn render(catalog: &Catalog, record: &AssessmentFile, opts: &Options) -> Res let mut printed_stimulus: Option = None; let layout = select::layout(record, &opts.form); - for (position, placement) in layout.iter().filter(|p| !p.dropped).enumerate() { + for (position, placement) in layout.iter().filter(|p| p.was_printed()).enumerate() { let entry = catalog.require(&placement.item)?; let item = &entry.item; let number = position + 1; @@ -602,6 +602,7 @@ items: credit_overrides: Default::default(), dropped: false, dropped_as: None, + dropped_before_printing: false, }, Placement { number: 2, @@ -616,6 +617,7 @@ items: credit_overrides: Default::default(), dropped: false, dropped_as: None, + dropped_before_printing: false, }, ], } diff --git a/src/export/qti.rs b/src/export/qti.rs index 86e31f9..cba14ef 100644 --- a/src/export/qti.rs +++ b/src/export/qti.rs @@ -1398,6 +1398,7 @@ items: credit_overrides: Default::default(), dropped: false, dropped_as: None, + dropped_before_printing: false, }, Placement { number: 2, @@ -1412,6 +1413,7 @@ items: credit_overrides: Default::default(), dropped: false, dropped_as: None, + dropped_before_printing: false, }, ], } diff --git a/src/export/site.rs b/src/export/site.rs index 986d102..669889d 100644 --- a/src/export/site.rs +++ b/src/export/site.rs @@ -867,6 +867,7 @@ items: credit_overrides: Default::default(), dropped: false, dropped_as: None, + dropped_before_printing: false, }, Placement { number: 2, @@ -881,6 +882,7 @@ items: credit_overrides: Default::default(), dropped: false, dropped_as: None, + dropped_before_printing: false, }, ], } diff --git a/src/export/typst.rs b/src/export/typst.rs index 7c4113d..a1762a9 100644 --- a/src/export/typst.rs +++ b/src/export/typst.rs @@ -440,6 +440,7 @@ mod tests { credit_overrides: Default::default(), dropped: true, dropped_as: None, + dropped_before_printing: false, }, Placement { number: 2, @@ -454,12 +455,13 @@ mod tests { credit_overrides: Default::default(), dropped: false, dropped_as: None, + dropped_before_printing: false, }, ], }; let printable: Vec = select::layout(&record, &Options::default().form) .into_iter() - .filter(|p| !p.dropped) + .filter(|p| p.was_printed()) .map(|p| p.number) .collect(); assert_eq!(printable, vec![2]); diff --git a/src/export/typst/templates/cohort-report.typ b/src/export/typst/templates/cohort-report.typ index c39353a..1e01e4a 100644 --- a/src/export/typst/templates/cohort-report.typ +++ b/src/export/typst/templates/cohort-report.typ @@ -1088,12 +1088,17 @@ #explain[ The full breakdown for every flagged question: what the flags mean, and who - chose what. The r column beside each option is the correlation between - choosing that option and scoring well on the rest of the exam, which is how a - defensible distractor announces itself. + chose what. In question order, so you can find one by its number; the lists + above are the same questions sorted by what they ask of you. The r column + beside each option is the correlation between choosing that option and + scoring well on the rest of the exam, which is how a defensible distractor + announces itself. ] - #for q in revise [ + // Question order here, not triage order. The lists above are for deciding + // what to do; this section is for looking one question up while you do it, + // and a reader with a number in hand should not have to scan every entry. + #for q in revise.sorted(key: q => q.number) [ #block(breakable: false, above: entry-gap, width: 100%)[ #{ let parts = ( diff --git a/src/model/assessment.rs b/src/model/assessment.rs index 455e639..b217535 100644 --- a/src/model/assessment.rs +++ b/src/model/assessment.rs @@ -305,6 +305,28 @@ pub struct Placement { /// contribute. #[serde(default, skip_serializing_if = "Option::is_none")] pub dropped_as: Option, + /// Set only when the item was pulled *before* the paper was printed. + /// + /// `dropped` on its own means what its own documentation says: the item was + /// printed, students answered it, and it was then taken out of scoring. + /// Such an item keeps its printed position, because it occupied one on the + /// page the students held, and the responses that come back are numbered + /// around it. + /// + /// An item pulled before printing never occupied a position, so every later + /// question moves up one. That case has to be distinguished, and it cannot + /// be inferred: both look identical in the record. Getting it wrong is not + /// a cosmetic error. Sealing a post-administration drop as if it had never + /// been printed renumbers every question after it, so each response is + /// attributed to the wrong item, the statistics for those items are + /// computed from answers to different questions, and nothing in the output + /// looks obviously wrong. + /// + /// Practically: leave this alone when you discover a bad question after the + /// exam, which is the common case. Set it when you cut a question from the + /// draft and reprinted. + #[serde(default, skip_serializing_if = "is_false")] + pub dropped_before_printing: bool, } /// How a dropped item was handled on the grading platform. @@ -327,6 +349,19 @@ impl Placement { pub fn dropped_with_credit(&self) -> bool { self.dropped && self.dropped_as == Some(DropStyle::FullCredit) } + + /// Whether this placement occupied a printed position on the paper. + /// + /// Everything that lays out a page or reads a page back goes through this, + /// so the printed form, the seal, and the decoder cannot disagree about + /// which question sat where. + /// + /// # Returns + /// + /// `true` unless the item was pulled before printing. + pub fn was_printed(&self) -> bool { + !(self.dropped && self.dropped_before_printing) + } } impl AssessmentFile { diff --git a/src/model/seal.rs b/src/model/seal.rs index f482e7a..b5c9236 100644 --- a/src/model/seal.rs +++ b/src/model/seal.rs @@ -518,12 +518,13 @@ fn sealed_item( fn sealed_form(catalog: &Catalog, record: &AssessmentFile, form: &Form) -> Result { let mut questions = Vec::new(); - // Dropped placements are not printed, so they take no printed position. A - // drop recorded before sealing therefore shifts every later position, exactly - // as it shifts them on the page. + // Only an item pulled before printing takes no printed position. An item + // dropped from scoring after the exam was on the page and keeps its place, + // so re-sealing after a drop describes the same paper the students held + // rather than renumbering everything after it. let printed: Vec = select::layout(record, form) .into_iter() - .filter(|p| !p.dropped) + .filter(|p| p.was_printed()) .collect(); for (index, placement) in printed.iter().enumerate() {