diff --git a/CHANGELOG.md b/CHANGELOG.md index abee3c9..0b3c570 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,14 @@ v0.5.1: no run before v0.5.1 is read.) **Fixed** +- **Notebook: a removal or rename answer confirms only the change it answers (#279).** A + re-confirmation names a channel ID, not a folder (experiment-v1 ยง4.6), so **Yes, remove** of + one folder accepted any change of its channel IDs, including a saved header name lost in + another folder that is still there. Such an ID now stays refused (`R_CONFIRMED_ID_CHANGED`) and + the line beside Save names the channel and the folder (undo that tag on Channels). A header + name held only by the removed folder is still the removal's, and a moved position still needs + its own click (#258). A header that now states no channel names at all counts as a lost name + too, and a folder rename answer does not accept channels re-ordered inside the renamed folder. - **Notebook: a channel re-confirmation after re-ordering a unit's channels (#258).** A saved unit re-tagged in another order (its images re-exported with the channels re-ordered) was refused with `R_CONFIRMED_ID_CHANGED` with no way forward. The Channels step now lists each diff --git a/src/morphobricks_preprocessing/description_reconcile.py b/src/morphobricks_preprocessing/description_reconcile.py index aa9ae4a..a895a6b 100644 --- a/src/morphobricks_preprocessing/description_reconcile.py +++ b/src/morphobricks_preprocessing/description_reconcile.py @@ -119,7 +119,10 @@ class UnitChange: ``repeat_id``, ``condition_id`` and ``technical_id`` are the published IDs of an unchanged or removed unit, None for a new one, which is pending. ``proposed_repeat`` is the only proposal - for a new unit: the published repeat ID equal to its source repeat label. + for a new unit: the published repeat ID equal to its source repeat label. ``headers_changed`` + are the saved channels of a unit still in the folder whose header, at their saved position, + no longer gives one of their saved header names (another name, or none): a change in this + folder that no removal or rename answer elsewhere confirms (issue #279). """ folders: UnitFolders @@ -130,6 +133,7 @@ class UnitChange: proposed_repeat: str | None = None reasons: tuple[str, ...] = () image_count: int | None = None + headers_changed: tuple[str, ...] = () @dataclass(frozen=True) @@ -245,6 +249,29 @@ def _channels_differ( return False +def _headers_changed( + published: PublishedUnit, seen: ObservedUnit, description: Description +) -> tuple[str, ...]: + """Return the unit's saved channels whose header name at their position is not a saved one. + + A position whose header now gives no name counts too when the channel has saved names: the + name it may have given is lost. Headers that state no names at all (or were not read) give + no saved name either, so they count the same: no name is known to be still given there. + A colour unit has no header names. + """ + unit = published.unit + if unit.image_type == "rgb": + return () + names = seen.header_names or () + out = set() + for index, channel in enumerate(unit.channels): + known = {_nfc(entry) for entry in description.channels[channel].header_names or ()} + name = names[index] if index < len(names) else None + if known and (not name or _nfc(name) not in known): + out.add(channel) + return tuple(sorted(out)) + + def _nfc(text: str) -> str: return unicodedata.normalize("NFC", text) @@ -319,7 +346,7 @@ def reconcile( found = [RENAMED_FOLDER] * (key in renamed) found += [CHANNELS_CHANGED] * _channels_differ(unit, seen, published) reasons[key].update(found) - changes.append(_change(unit, Status.UNCHANGED, tuple(found), current, seen)) + changes.append(_change(unit, Status.UNCHANGED, tuple(found), current, seen, published)) known = set(now.values()) pending = [key for key in observed if key not in known] changes += [_new(key, observed[key], published, now, reasons, units) for key in sorted(pending)] @@ -336,7 +363,9 @@ def _change( reasons: tuple[str, ...], folders: UnitFolders | None = None, seen: ObservedUnit | None = None, + description: Description | None = None, ) -> UnitChange: + """Return one published unit's change (``seen`` and ``description`` for a unit still there).""" return UnitChange( folders or unit.folders, status, @@ -346,6 +375,7 @@ def _change( None, tuple(sorted(reasons)), seen.image_count if seen else None, + _headers_changed(unit, seen, description) if seen and description else (), ) diff --git a/src/morphobricks_preprocessing/description_removal.py b/src/morphobricks_preprocessing/description_removal.py index bce9817..9fcea53 100644 --- a/src/morphobricks_preprocessing/description_removal.py +++ b/src/morphobricks_preprocessing/description_removal.py @@ -43,6 +43,9 @@ Change = tuple[str, str, str | None] """A changed confirmed ID as ``Reconfirmation`` names it: (kind, old ID, new ID or None).""" +POSITION, HEADER_NAME = "position", "header name" +"""What a channel lost in a folder still there (:attr:`Reconfirmations.unseen`).""" + @dataclass(frozen=True) class RemovalQuestion: @@ -81,10 +84,16 @@ def __post_init__(self) -> None: @dataclass(frozen=True) class Reconfirmations: - """The records the candidate needs: those the user's answers cover, and those they do not.""" + """The records the candidate needs: those the user's answers cover, and those they do not. + + ``unseen`` are the channel changes in folders still there that keep an ID open although an + answer touches it: each the channel ID, the folder and what it lost there (``POSITION`` or + ``HEADER_NAME``; issue #279). + """ accepted: tuple[Reconfirmation, ...] open: tuple[TransitionIssue, ...] + unseen: tuple[tuple[str, UnitFolders, str], ...] = () def _moves( @@ -236,6 +245,52 @@ def candidate( return built +def _unanswered( + issue: TransitionIssue, + published: Description, + built: Description, + rec: Reconciliation, + removed: set[UnitFolders], + now: dict[UnitFolders, UnitFolders], +) -> set[tuple[UnitFolders, str]]: + """Return the folders still there where a channel changed in a way no answer covers. + + Section 4.6 keys a re-confirmation by ID only: a removal or rename answer that touches a + channel would also accept its other changes. Those are a position lost in a unit no answer + names (a move, confirmed on its own, #258) and a lost header name while a folder still there + no longer gives a saved name at the channel's position (#279). A position is compared in + the unit's folders after the renames (``now``), so a move inside a renamed folder is lost + too; only a unit the user confirmed removed (``removed``) loses its positions by the answer. + """ + if issue.kind != CHANNELS or issue.new_id is not None: + return set() + channel, units = issue.old_id, published_units(published) + by_ids = {(u.condition_id, k.source_repeat, u.technical_id): k for k, u in units.items()} + after = _positions_of(built, channel) + lost = { + key + for cond, repeat, tid, index in _positions_of(published, channel) + if (key := by_ids[cond, repeat, tid]) not in removed + and (cond, now[key].source_repeat, tid, index) not in after + } + out = {(now[key], POSITION) for key in lost} + names = set(published.channels[channel].header_names or ()) + if not names <= set(built.channels[channel].header_names or ()): + out |= {(u.folders, HEADER_NAME) for u in rec.unchanged if channel in u.headers_changed} + return out + + +def _positions_of(description: Description, channel: str) -> set[tuple[str, str, str, int]]: + """Return the positions ``(condition, source repeat, technical, index)`` of one channel.""" + return { + (cond, member.source_repeat, tid, unit.channels.index(channel)) + for repeat in description.repeats.values() + for cond, member in repeat.members.items() + for tid, unit in member.technical_repeats.items() + if channel in unit.channels + } + + def reconfirmations( published: Description, built: Description, @@ -245,21 +300,26 @@ def reconfirmations( """Return the re-confirmations of ``built`` that the user's answers cover, and the rest. Each changed confirmed ID takes the date of the first answer that touches one of its units; - an ID no answer touches is in ``open`` (the candidate would be refused for it). + an ID no answer touches is in ``open`` (the candidate would be refused for it). A channel + that also changed in a folder still there in a way no answer covers is open as well, with + those folders in ``unseen``: the record cannot say "this ID, only in that folder" (4.6). """ units = published_units(published) answers = [(set(r.units), r.confirmed_on) for r in removals] + removed = {key for held, _ in answers for key in held} now = {key: key for key in units} for rename in rec.renames: after = {key: renamed_key(current, rename) for key, current in now.items()} answers.append(({key for key in units if after[key] != now[key]}, rename.confirmed_on)) now = after - accepted, left = set(), [] + accepted, left, unseen = set(), [], set() for issue in _changes(published, built): reached = _affected(issue, units) dates = [day for held, day in answers if held & reached] - if dates: + others = _unanswered(issue, published, built, rec, removed, now) if dates else set() + if dates and not others: accepted.add(Reconfirmation(issue.kind, issue.old_id, issue.new_id, dates[0])) else: left.append(issue) - return Reconfirmations(tuple(sorted(accepted)), tuple(left)) + unseen |= {(issue.old_id, *other) for other in others} + return Reconfirmations(tuple(sorted(accepted)), tuple(left), tuple(sorted(unseen))) diff --git a/src/morphobricks_preprocessing/notebook_channel_moves.py b/src/morphobricks_preprocessing/notebook_channel_moves.py index 45d7faa..dc9bc9c 100644 --- a/src/morphobricks_preprocessing/notebook_channel_moves.py +++ b/src/morphobricks_preprocessing/notebook_channel_moves.py @@ -11,7 +11,7 @@ from __future__ import annotations import html -from dataclasses import replace +from dataclasses import astuple, replace from datetime import date import ipywidgets as widgets @@ -19,6 +19,7 @@ from . import notebook_style as ui from .channel_moves import ChannelMove, channel_moves, move_confirmations from .description import Description +from .description_removal import reconfirmations from .description_transition import Reconfirmation from .draft import DraftError from .draft_answers import description_from_draft @@ -91,6 +92,30 @@ def _move_terms(self, state: object, candidate: Description) -> tuple[Reconfirma clicked = move_confirmations(self._moves_of(state, candidate), self._moves_clicked) return tuple(c for c in clicked if (c.kind, c.old_id) not in taken) + def _unseen_lines(self) -> list[str]: + """Say which channel changed in a folder still there that no answer confirms (#279). + + A removal or rename answer re-confirms a channel ID, never one folder of it, so that + change keeps the ID refused until it is undone; nothing is confirmed for the user. + """ + state, check = self._published(), self._check + if state is None or check is None or check.description is None: + return [] + candidate = check.description + found = reconfirmations( + state.description, + candidate, + state.reconciliation, + self._removal_terms(state, candidate)[0], + ) + kept = {c.old_id for c in self._publication_terms(state, candidate)[1]} + return [ + f"{channel} lost a saved {what} in {_where(astuple(folders))}, which is still there:" + " a removal or rename does not confirm it (undo that tag on Channels)" + for channel, folders, what in found.unseen + if channel not in kept + ] + def _refresh_channel_moves(self) -> None: """Show the moved channels of the answers now and whether the user confirmed them.""" moves: tuple[ChannelMove, ...] = () diff --git a/src/morphobricks_preprocessing/notebook_check_save.py b/src/morphobricks_preprocessing/notebook_check_save.py index 6e3409e..9fb509b 100644 --- a/src/morphobricks_preprocessing/notebook_check_save.py +++ b/src/morphobricks_preprocessing/notebook_check_save.py @@ -250,7 +250,9 @@ def _save_blockers(self, todo: tuple[list, list[str]]) -> list[str]: return [f"{plural(missing, 'answer')} still missing (listed above the button)"] if self._moves_open(): return ["the moved channels are not confirmed: see Channels"] - return ["these answers are refused: see the red message above the button"] + return self._unseen_lines() or [ + "these answers are refused: see the red message above the button" + ] def _blocked(self, state: object) -> str: """Say what already stops the run while the candidate cannot be checked (answers missing). diff --git a/tests/test_reconfirm_scope.py b/tests/test_reconfirm_scope.py new file mode 100644 index 0000000..8d48dab --- /dev/null +++ b/tests/test_reconfirm_scope.py @@ -0,0 +1,107 @@ +"""A removal or rename answer re-confirms only the change it answers (issue #279). + +Experiment-v1 4.6 keys a re-confirmation by ID (old ID, new ID, date), never by folder: the +"Yes, remove" of one folder records every channel ID that folder held, and that record would +accept any other change of the same ID, such as a saved header name lost in another folder that +is still there and that the user never saw. Such an ID stays refused (``R_CONFIRMED_ID_CHANGED``) +and the reason is said beside the Save button; a header name held only by the removed folder is +still the removal's. +""" + +import json +import shutil + +import pytest + +from channel_images import write_channels +from morphobricks_preprocessing.description_file import RECORDS_FILENAME +from test_notebook_check_save import KINDS, answer_all, exp # noqa: F401 + +pytest.importorskip("ipywidgets") +pytest.importorskip("matplotlib") + +from test_notebook_axes_quick import stepper # noqa: E402 +from test_notebook_prerelease import _tag, _text # noqa: E402 + +GONE = "drug/rep2" +NO_NAMES = object() +"""The rewritten header states no channel name at all (not one empty name).""" + + +def _reopened(exp, unique, now): # noqa: F811 + """Save with ``unique`` as ctrl/rep1's DNA header name, then rewrite it to ``now``; drop B.""" + write_channels(exp[0] / "ctrl/rep1/a.tif", [unique, "Actin", "Ch3"], KINDS, seed=1) + first = stepper(*exp) + first.quick_confirm.click() + answer_all(first) + assert first.save_experiment() + assert first.draft_answers.channels["DNA"].header_names == ("DAPI", unique) + names = None if now is NO_NAMES else [now, "Actin", "Ch3"] + write_channels(exp[0] / "ctrl/rep1/a.tif", names, KINDS, seed=1) + shutil.rmtree(exp[0] / GONE) + shutil.rmtree(exp[1] / "Nuclei" / GONE) + nb = stepper(*exp) + nb.quick_confirm.click() + return nb + + +def _remove(nb): + answer_all(nb) + (row,) = nb.removals_box.children + row.children[1].click() # Yes, remove (drug/rep2 holds DNA too) + nb._refresh_check_save() + + +def _records(exp): # noqa: F811 + path = exp[2] / RECORDS_FILENAME + return [json.loads(x) for x in path.read_text().splitlines()] if path.exists() else [] + + +@pytest.mark.parametrize( + "now", ["Nuc", None, NO_NAMES], ids=["other-name", "no-name", "no-header-names"] +) +def test_a_removal_does_not_accept_a_header_name_lost_in_a_folder_still_there(exp, now): # noqa: F811 + nb = _reopened(exp, "Hoechst", now) + before = len(_records(exp)) + _tag(nb, 0, "DNA") # re-tag ctrl/rep1's channel 1: Hoechst is no longer a name of DNA + assert "Hoechst" not in (nb.draft_answers.channels["DNA"].header_names or ()) + _remove(nb) + assert nb.save_button.disabled and not nb.save_experiment() + assert "R_CONFIRMED_ID_CHANGED at channels.DNA" in _text(nb.checks.value) + assert ( + "DNA lost a saved header name in ctrl/rep1, which is still there: a removal or rename " + "does not confirm it (undo that tag on Channels)" + ) in _text(nb.save_blocker.value) + assert len(_records(exp)) == before # nothing published, nothing recorded + + +def test_without_the_tag_change_the_removal_saves(exp): # noqa: F811 + # The way out the reason names: DNA keeps its saved names, so only the removal changes it + # (the run then stops on the header itself, R_HEADER_CONTRADICTION, as without a removal). + nb = _reopened(exp, "Hoechst", "Nuc") + _remove(nb) + assert not nb.save_button.disabled + assert nb.save_experiment(), _text(nb.save_status.value) + kept = {r["old_id"] for r in _records(exp) if r["type"] == "reconfirmation"} + assert {"ACTIN", "DNA", "PHASE"} <= kept + + +def test_a_header_name_held_only_by_the_removed_folder_is_the_removals(exp): # noqa: F811 + # drug/rep2 alone named DNA "Hoechst"; with it removed, a re-tag drops that name. Every + # folder still there gives a saved name, so the removal answer covers the lost name. + write_channels(exp[0] / GONE / "a.tif", ["Hoechst", "Actin", "Ch3"], KINDS, seed=4) + first = stepper(*exp) + first.quick_confirm.click() + answer_all(first) + assert first.save_experiment() + shutil.rmtree(exp[0] / GONE) + shutil.rmtree(exp[1] / "Nuclei" / GONE) + nb = stepper(*exp) + nb.quick_confirm.click() + _tag(nb, 0, "DNA") + assert nb.draft_answers.channels["DNA"].header_names == ("DAPI",) + _remove(nb) + assert not nb.save_button.disabled, _text(nb.save_blocker.value) + assert nb.save_experiment(), _text(nb.save_status.value) + kept = [r["old_id"] for r in _records(exp) if r["type"] == "reconfirmation"] + assert kept.count("DNA") == 1 diff --git a/tests/test_spec_reconcile.py b/tests/test_spec_reconcile.py index e38f672..7816cf7 100644 --- a/tests/test_spec_reconcile.py +++ b/tests/test_spec_reconcile.py @@ -417,6 +417,33 @@ def test_a_member_folder_rename_is_stated_under_its_condition(): assert ("repeats", "Day1") in {(c.kind, c.old_id) for c in found.accepted} +def test_a_member_folder_rename_does_not_accept_a_move_inside_that_folder(): + # Issue #279: the rename answer re-confirms DNA and ACTIN by ID (4.6). Channels re-ordered + # inside the renamed folder are a move the user never saw: those IDs stay open. + published = model("v03") + statement = FolderRename("member", "experiment_23", "experiment_23b", DAY, condition="DrugA") + rec = reconcile(published, renamed_member_folder(published), [statement]) + draft = candidate(published, rec) + member = draft.repeats["Day1"].members["DrugA"] + unit = member.technical_repeats["single"] + assert unit.channels == ("DNA", "ACTIN") + swapped = replace( + member, technical_repeats={"single": replace(unit, channels=("ACTIN", "DNA"))} + ) + day1 = replace( + draft.repeats["Day1"], members={**draft.repeats["Day1"].members, "DrugA": swapped} + ) + moved = replace(draft, repeats={**draft.repeats, "Day1": day1}) + found = reconfirmations(published, moved, rec) + assert {(c.kind, c.old_id) for c in found.accepted} & { + ("channels", "DNA"), + ("channels", "ACTIN"), + } == set() + where = key("DrugA", "experiment_23b") + assert found.unseen == (("ACTIN", where, "position"), ("DNA", where, "position")) + assert reconfirmations(published, draft, rec).unseen == () # the rename alone: nothing open + + def test_a_technical_folder_rename_is_stated_by_its_folder_label(): published = model("v06") units = published_units(published)