Repository navigation
feat: read nuclei data from looptrace pipeline's B03_NUCLEI_SEGMENTAT… - #5
Conversation
…ION folder The looptrace Nextflow pipeline publishes nuclei segmentation outputs into Analysis_<NNN>_<timestamp>/B03_NUCLEI_SEGMENTATION/, where the centroids folder is named nuclear_masks_visualisation (no underscore prefix). Accept that name, preferring it over the legacy _nuclear_masks_visualisation when both are present, so the B03 folder can be dropped directly into napari. - add tests for the pipeline layout, incl. the .zgroup at the nuc_images root - update user docs: drop the B03 folder; the dropped folder's name isn't checked - add changelog entry
Review: reading nuclei data from
|
get_reader checked that the three subfolders exist, not that they describe any field of view in common. Returning a reader is a claim that the folder can be read, so napari accepted the drop and the parse then died inside np.stack on an empty list -- a message naming neither fields of view nor the folder, for a user who had done nothing stranger than drag a partial-FOV run. Reachable whenever the folders describe different subsets: a run restricted with selected_fovs, which the documentation now advertises, or three folders assembled by hand from different analyses. The intersection moves to NucleiDataSubfolders.shared_fields_of_view, built on paths_by_fov, and read_all_from_root uses the same thing -- one definition of "usable field of view" rather than two that could drift. Both work from filenames, so the check costs a directory listing rather than an array read, which is what makes it cheap enough for get_reader. The refusal names the counts per subfolder, since three non-empty folders with no overlap is otherwise a puzzling thing to be told about. build_layers keeps a guard for empty input: unreachable through get_reader now, but it explains itself in terms of fields of view if it is ever reached. Three tests, each verified to fail without the change.
The fixture wrote a .zgroup into nuc_images/, commented "the published images folder is a zarr group". It is not: looptrace writes per-FOV stores into a plain directory, checked against four published analysis folders including the first produced by the code that added this output. The test passed anyway, because discovery selects entries by parsing a field of view out of the name and skips what it cannot parse -- so it passed for a reason unrelated to what its comment claimed, and would have kept passing if the reader came to depend on group metadata real data lacks. The tolerance being relied on is worth keeping, so it becomes a test of its own rather than a silent fixture detail: a stray .zgroup and a .DS_Store in the images folder must not stop the read. Nothing guarantees that folder holds only <fov>.zarr entries -- zarr tooling writes group metadata, filesystems leave clutter -- and now that is asserted on purpose.
The refusal listed all three expected paths whatever was wrong, so an analysis folder produced before looptrace published nuc_images -- two correct subfolders and one absent, which will be the commonest refusal for a while -- read like a malformed folder rather than an old one, and said nothing about the way out. It now names what is missing, and when that is nuc_images it says the pipeline publishes it only in newer versions and that resuming the run republishes it from cached task output. Six tests pinned the previous wording. That is a real contract, so they are updated rather than the message softened: a subfolder dropped in place of the parent must still report all three as missing. CHANGELOG moves from [Unreleased] -- a heading this file has never used, every prior section being versioned and dated -- to [v0.2.0], with pyproject.toml and __init__.py bumped to match. v0.2.0 rather than v0.1.10 because looptrace's development log already names v0.2.0 as the pin to move to, and reading a new published layout is a feature. The section also gains the two behaviour changes from the preceding commits, which would otherwise ship unmentioned.
…_reader
Review points 5, 6 and 7. None of this changes what the plugin does; it
closes the gap between what the reader promises and what the tests check.
7. Every existing reader test calls get_reader directly, which proves the
function accepts a folder -- not that napari ever offers this plugin for
one. That routing is the documented claim ("the name of the folder to
drop doesn't matter"), and it rests entirely on npe2 treating directories
specially: a reader declaring accepts_directories is indexed under an
empty pattern, and a directory path yields exactly those readers, with
filename_patterns never consulted. Nothing in the plugin enforces that, so
a manifest edit could have broken the claim with every other test still
green. tests/test_napari_dispatch.py now registers the manifest with
npe2.PluginManager and asserts the dispatch itself: an example folder
reaches this reader, a folder named B03_NUCLEI_SEGMENTATION does too
(the name really is irrelevant), and a plain file does not. Guarded with
importorskip, since npe2 arrives with napari, which this project declares
only for macOS -- the other two CI platforms skip rather than fail.
5. napari.yaml now says why filename_patterns is vestigial here and must stay
narrow. It gates nothing today, but widening it to '*' would not make it
honest: it would offer this plugin for every FILE dropped into napari,
which get_reader then declines, putting a useless entry in the
reader-choice dialog.
6. NucleiDataSubfolders.relpaths is gone; its only caller disappeared when the
refusal message stopped listing all three subfolders. relpath's docstring
described a path join, which is not what it does -- the centers subfolder
has two accepted spellings, so which path is correct is a question about
the filesystem, and it probes for it, falling back to the preferred
spelling so a caller reporting an absence still has a name to print.
Also ran ruff format over the tree: my earlier commits in this branch wrapped
at 88 columns, and this project's format check runs against a longer line, so
CI would have failed on files already merged here.
The importorskip guard I added was justified by npe2 arriving via napari, which this project declares only for darwin. That is wrong: npe2 is a direct, unmarked dependency of the testsuite group, which is why test_manifest_validation.py has imported it unguarded on all three platforms all along. Harmless in effect -- npe2 is present everywhere, so the test ran everywhere -- but wrong in kind. A skip guard on the one test that catches a broken manifest means a missing npe2 turns it silently green, which is the failure mode the test exists to prevent. Also checked against npe2 0.7.9, the version poetry.lock pins, rather than only the 0.8.3 in my local environment: register, iter_compatible_readers and PluginManifest.from_file are all present in 0.7.9 and the suite passes on both.
… selection Review of the branch before pushing. Two of these would have failed CI, and one is a regression this branch introduced. Doubled @classmethod on NucleiDataSubfolders.paths_by_fov. It runs today only because CPython tolerated chaining classmethod descriptors, deprecated in 3.11 and removed in 3.13; the python = ">=3.10,<3.13" cap is all that hides it, and ruff has no rule for it. It would have detonated the day someone lifted the cap. get_reader could raise instead of returning None. Checking fields of view eagerly -- correct in itself, since returning a reader is a claim the folder can be read -- moved gertils' duplicate-FOV RuntimeError into napari's reader SELECTION path, where an exception is a crash rather than a decline that hands the drop to the next plugin. Reachable because names are parsed with lstrip("P")/rstrip(ext), so P1.zarr beside P0001.zarr both parse to 1. Caught and declined, with a test; the stray-entry test covered unparsable names but not colliding ones. codespell, which runs in the format job, rejected "unparseable" (exit 65). all_present_within had lost its last caller when get_reader inlined the missing comprehension, exactly as relpaths had. Both are now recorded as removed. One scan instead of four. shared_fields_of_view now takes the paths_by_fov result rather than a path, so get_reader and read_all_from_root each list the subfolders once, and the per-subfolder counts are built only for the message that needs them. The folder checks move into _why_not_readable: the try/except pushed get_reader past C901 (11 > 10) and PLR0911 (7 > 6), and a helper returning a reason beats a noqa on a function that had grown four log-and-return pairs. Also: the refusal no longer renders a period against the log's comma; the folder-preference test now pins the centroids the user sees rather than only the chosen path, and fails if the preference is inverted; and the changelog uses Keep a Changelog sections instead of an invented "Internal" one.
Review follow-up. Catching only RuntimeError left the same escape hatch open one step over: a subfolder that exists but cannot be listed passes is_present_within, since is_dir only stats, and then raises PermissionError inside the listing. On group-shared analysis storage with mixed ownership that is likelier than the padding collision, and a stale NFS handle on a flaky mount is likelier still. Both mean "the filesystem will not describe this folder", which is a decline, not a bug to propagate. The catch is widened to (RuntimeError, OSError), and the guard moves to wrap the whole check rather than just the scan, because there are two raising shapes, not one. pathlib does not swallow EACCES -- _ignore_error covers ENOENT, ENOTDIR, EBADF and ELOOP -- so is_dir RE-RAISES it instead of reporting "not a directory". A dropped folder that is readable but not traversable therefore throws while merely checking which subfolders exist, before anything is listed. Verified with real permission bits: both shapes previously escaped get_reader, both now decline, and a healthy folder still reads. The guard sits in get_reader rather than in the helper because not raising is get_reader's contract with napari; the helper stays a predicate. Refusals now carry a level. A folder that has all three subfolders but still cannot be read is almost certainly the one the user meant, and napari reports only "no reader available" -- hiding a cause they could act on, a permission bit or a duplicated field of view, behind a message about the plugin. All three post-gate refusals are WARNING; do_not_parse has always taken a level and nothing had ever passed one. Declining a folder that is not nuclei data stays silent, since every plugin is asked about every drop. Tested by injection rather than chmod(0o000), which does nothing useful on Windows and silently stops working as root, so a permissions-based test would pass vacuously on exactly the runners that differ from a laptop. Both tests fail if OSError is dropped from the catch.
CI's codespell (2.4.1, per poetry.lock) rejects "lets" as "let's"; the copy I checked locally was newer and does not, which is why this was reported clean. Same sentence, wording both versions accept -- and the phrasing the PR description already uses.
MergingGreen across all three workflows — 83 tests on Python 3.10/3.11/3.12 × Linux, This started as a reader that accepted the pipeline's folder layout and ends as Refusals reached after all three subfolders are found are logged at WARNING. Two things are pinned by tests that did not exist before: napari's dispatch of a Released as v0.2.0, the version looptrace's development log names as the pin Reviewed in three passes, the last two over my own changes; the decline paths |
Summary
nuclear_masks_visualisation(as published by the looptrace Nextflow pipeline) in addition to the legacy_nuclear_masks_visualisation; the unprefixed name wins if both exist.Analysis_<NNN>_<timestamp>/B03_NUCLEI_SEGMENTATION/into napari (requires looptrace to publishnuc_imagesthere).imageswas wrong (npe2 ignoresfilename_patternsfor directories).Testing
tests/test_pipeline_layout.py; full suite passes.Edit (@cchlanger): seven commits added on review
The original commit is unchanged; everything below sits on top of it. The premise checks out against looptrace itself —
docs/pipeline/outputs.mddocuments exactly this layout and says the underscore copy "is not published; the CSVs to use are the ones inB03_NUCLEI_SEGMENTATION/nuclear_masks_visualisation/" — so preferring the unprefixed name matches upstream guidance rather than guessing. So does thefilename_patternscorrection: npe2 indexes a reader declaringaccepts_directoriesunder an empty pattern and yields exactly those for a directory path, never consulting the patterns.Released as v0.2.0.
CHANGELOG.mdhad gained an[Unreleased]heading, which this file has never used — every prior section is versioned and dated. Now[v0.2.0], withpyproject.tomland__init__.pybumped to match. v0.2.0 rather than v0.1.10 because looptrace's development log already names v0.2.0 as the pin to move to, and reading a new published layout is a feature.Two behaviour fixes in the reader, both about the same contract: returning a reader is a claim that the folder can be read, and declining must not throw.
selected_fovs, or subfolders assembled by hand from different analyses — was accepted and then died insidenumpy.stackon an empty list, naming neither fields of view nor the folder.RuntimeErrorwhen two filenames parse to one field of view (P1.zarrbesideP0001.zarr, since names are parsed withlstrip("P")/rstrip(ext)), andOSErrorwhen the filesystem will not describe a folder — a subfolder present but unreadable, or a stale handle on a network mount.pathlibre-raises EACCES rather than reporting "not a directory", so the guard wraps the whole check, not just the listing. The second case was reachable before this PR too, at parse time.Refusals now say something useful. The message named all three expected paths whatever was wrong, so an analysis folder produced before looptrace published
nuc_images— two correct subfolders and one absent, which will be the commonest refusal for a while — read like a malformed folder rather than an old one, and said nothing about the way out. It now names what is missing and, fornuc_images, says that resuming the run republishes it from cached task output. Refusals reached after all three subfolders were found are logged at WARNING rather than DEBUG: that folder is almost certainly the one the user meant, and napari reports only "no reader available", hiding a cause they could act on. Declining folders that are not nuclei data at all stays silent, since every plugin is asked about every drop.Tests.
tests/test_napari_dispatch.pyasserts the routing itself throughnpe2's dispatch — an example folder reaches this reader, a folder namedB03_NUCLEI_SEGMENTATIONdoes too, a plain file does not. Every other reader test callsget_readerdirectly, which proves the function accepts a folder, not that napari ever offers this plugin for one; that promise rests entirely onaccepts_directories, which no test exercised. The pipeline fixture also stopped writing a.zgroupintonuc_images/: looptrace writes per-FOV stores into a plain directory (checked against four published analysis folders), so the fixture was testing a layout the pipeline does not produce. Stray entries are now covered by their own test instead.Cleanups.
NucleiDataSubfolders.relpathsandall_present_withinare gone, both without callers once the refusal stopped listing all three subfolders. A folder is listed once per subfolder during selection rather than four times.napari.yamlrecords whyfilename_patternsis vestigial here and must not be widened to'*', which would offer this plugin for every file dropped into napari only forget_readerto decline it.83 tests pass;
ruff format,ruff checkandcodespellare clean. The decline paths were also checked against real permission bits rather than only the injected fakes the tests use.