Plugin fixes

.claude/skills/pre-release/SKILL.md

@@ -15,17 +15,34 @@ Execute the sections in order. After each, capture pass/fail and any findings. A
 
 ### Section 1 — API modules present in `docs/source/api.md`
 
-For every `src/ptychodus/api/*.py` file (excluding `_*.py` private modules), confirm `docs/source/api.md` contains a matching `.. automodule:: ptychodus.api.<stem>` directive.
+For every `src/ptychodus/api/**/*.py` file, confirm `docs/source/api.md` contains a matching `.. automodule:: ptychodus.api.<dotted.name>` directive. The scan is recursive, so subpackage modules (`preprocess/`, `simulate/`) are covered under their dotted names. A path component starting with `_` is skipped at any depth — that covers `__init__.py`, `simulate/_phase_unwrap.py`, and any future private subpackage.
 
-```sh
-# Modules that should be documented:
-ls src/ptychodus/api/*.py | xargs -n1 basename | sed 's/\.py$//' | grep -v '^_' | grep -v '^__' | sort
+Do not enumerate with a flat `ls src/ptychodus/api/*.py`, and do not match the directive with a `[a-z_]+` character class: the former misses every subpackage module, and the latter has no `.` so `preprocess.diffraction` truncates to `preprocess`. The two errors cancel out and the section reports `PASS` while blind to an undocumented subpackage module.
 
-# Modules currently documented:
-grep -oE '\.\. automodule:: ptychodus\.api\.[a-z_]+' docs/source/api.md | sed 's|.*ptychodus\.api\.||' | sort
+```sh
+uv run python -c "
+import pathlib, re
+root = pathlib.Path('src/ptychodus/api')
+have = set()
+for p in sorted(root.rglob('*.py')):
+    rel = p.relative_to(root).with_suffix('')
+    if any(part.startswith('_') for part in rel.parts):
+        continue
+    have.add('.'.join(rel.parts))
+doc = set(re.findall(r'\.\. automodule:: ptychodus\.api\.([A-Za-z_][A-Za-z_0-9.]*)',
+                     pathlib.Path('docs/source/api.md').read_text()))
+print(f'modules={len(have)} documented={len(doc)}')
+for m in sorted(have - doc):
+    print(f'MISSING FROM DOCS: ptychodus.api.{m}')
+for m in sorted(doc - have):
+    print(f'STALE DOC ENTRY:   ptychodus.api.{m}')
+print('PASS' if have == doc else 'FAIL')
+"
 ```
 
-Compute the diff. `PASS` if empty. `FAIL` with the list of missing module names.
+`PASS` if the script prints `PASS`. `FAIL` with the list of `MISSING FROM DOCS` and `STALE DOC ENTRY` lines.
+
+A `STALE DOC ENTRY` is a module that `api.md` documents but that no longer exists. Sphinx catches this downstream as an autodoc import failure under Section 6's `-W`, but naming it here gives a far better message. The fix is to delete the orphaned stanza from `docs/source/api.md`.
 
 **Suggested fix per missing module**: append this stanza to `docs/source/api.md`, matching the existing pattern (title-case `##` heading, then the autodoc directive as raw reStructuredText inside an `{eval-rst}` block):
 
@@ -85,41 +102,63 @@ print(f'--- {len(missing)} missing ---')
 
 ### Section 3 — Reader plugins represented in `docs/source/readers.md`
 
-`docs/source/readers.md` is a curated bullet list, not a 1:1 file mapping. Cross-check by keyword: for every distinct beamline/format hint in `src/ptychodus/plugins/`, ensure the readers doc mentions it.
-
-Keyword map (edit as new plugins land):
-
-| Plugin filename substring | Expected mention in readers.md |
-| --- | --- |
-| `aps02id_` | "2-ID-D Bionanoprobe" or "2-ID-D Microprobe" or "2-ID-E Microprobe" |
-| `aps04id_polar_` | "4-ID" and "Polar" (Polarization Modulation Spectroscopy) |
-| `aps09id_cssi_` | "9-ID" and "CSSI" |
-| `aps12id_` | "12-ID" and "SAXS" |
-| `aps19id_isn_` | "19-ID" and "ISN" |
-| `aps31id_lynx_` | "31-ID" and "LYNX" |
-| `aps33id_velociprobe` | "33-ID" and "Velociprobe" |
-| `lcls_` | "LCLS" |
-| `max_iv_nanomax_` | "MAX IV" or "NanoMAX" |
-| `fold_slice_` | "fold_slice" |
-| `cxi_` | "CXI" or "*.cxi" |
-| `csv_` | "CSV" or "Comma-Separated" |
-| `mda_` | "MDA" or "*.mda" |
-| `npy_` | "NumPy" or "*.npy" |
-| `delimited_position_` | "Space-Separated" or "*.txt" |
+`docs/source/readers.md` is a curated bullet list of user-facing names, not a 1:1 file mapping. Cross-check it against the plugin registry rather than against filenames: every plugin already declares a user-facing `display_name` in its `register_plugins` hook, and those strings are the authoritative source.
 
-```sh
-# List all plugin file stems:
-ls src/ptychodus/plugins/*.py | xargs -n1 basename | sed 's/\.py$//'
+Drive the check from `PluginRegistry.load_plugins()` so it reuses the exact discovery path the application uses. This matters because `pkgutil.iter_modules` is non-recursive: it yields `aps33id_velociprobe` as a *package* and calls the `register_plugins` in its `__init__.py`. A flat `ls src/ptychodus/plugins/*.py` misses that package entirely, and a hand-maintained keyword table drifts silently as new plugins land — do not reintroduce either.
 
-# For each keyword above, verify readers.md contains the expected mention:
-for kw in "2-ID-D Bionanoprobe" "4-ID" "9-ID" "12-ID" "19-ID" "31-ID" "33-ID" "LCLS" "NanoMAX" "fold_slice" "CXI" "CSV" "MDA" "NumPy" "Space-Separated"; do
-    grep -q "$kw" docs/source/readers.md || echo "MISSING: $kw"
-done
+Comparison is by facility token and beamline token, in two tiers:
+
+- **FAIL** — a token a plugin registers that `readers.md` never mentions. This is the real drift risk: a new beamline reader lands and the doc is not updated.
+- **REVIEW** — a token the doc claims that no plugin display name mentions. Report-only, because the doc legitimately uses friendlier names than the plugins do. Making this a hard failure would produce false alarms.
+
+```sh
+uv run python -c "
+import logging, re, pathlib
+
+skipped = []
+class _H(logging.Handler):
+    def emit(self, record):
+        m = record.getMessage()
+        if m.startswith(('Skipping ', 'Failed to register ')):
+            skipped.append(m)
+_lg = logging.getLogger('ptychodus.api.plugins')
+_lg.addHandler(_H())
+_lg.setLevel(logging.WARNING)
+
+from ptychodus.api.plugins import PluginRegistry
+registry = PluginRegistry.load_plugins()
+names = {p.display_name
+         for a in vars(registry) if a.endswith('_file_readers')
+         for p in getattr(registry, a)}
+doc = pathlib.Path('docs/source/readers.md').read_text()
+
+FACILITIES = ['APS', 'CNM', 'LCLS', 'MAX IV', 'NSLS-II', 'SLAC', 'SLS']
+BEAMLINE = re.compile(r'\b\d+-ID\b')
+
+def tokens(text):
+    found = {f for f in FACILITIES if re.search(rf'\b{re.escape(f)}\b', text)}
+    return found | set(BEAMLINE.findall(text))
+
+plug = tokens(' | '.join(names))
+docs = tokens(doc)
+print(f'reader plugins registered: {len(names)}; modules skipped at load: {len(skipped)}')
+for m in skipped:
+    print(f'  {m}')
+fails = sorted(plug - docs)
+for t in fails:
+    print(f'FAIL   in plugins, missing from readers.md: {t}')
+for t in sorted(docs - plug):
+    print(f'REVIEW in readers.md, no plugin names it: {t}')
+print('PASS' if not fails else 'FAIL')
+"
 ```
 
-Also flag *the reverse*: any bullet in `readers.md` whose beamline has no matching plugin file — that indicates a stale doc entry.
+Two details in that script are load-bearing:
+
+- Facility matching uses `\b...\b`, not substring. A plain `'SLS' in text` also matches **NSLS-II**, which would silently mask a missing SLS entry.
+- The script reports `modules skipped at load`. An optional-dependency plugin that fails to import registers nothing, which shrinks the plugin token set and weakens the check in the safe direction — under-reporting, never a false `FAIL`. Run the release gate in an environment with the full extras, and treat a nonzero skip count as a caveat on this section's coverage.
 
-`PASS` if every plugin has a doc mention and every doc mention has a plugin. `FAIL` with lists of orphans in either direction.
+`PASS` if the script prints `PASS` (no `FAIL` lines). `REVIEW` lines do not fail the gate, but report them so a human can adjudicate.
 
 **Suggested fix per finding**: propose a bullet insertion under the correct facility heading, matching the existing style (`- <Beamline name> (<abbreviation>)`). Ask the user to confirm the heading and wording — the doc uses friendly names, not filenames.
 

docs/source/readers.md

@@ -12,7 +12,6 @@ File readers are implemented using a Python namespace plugin system. We would be
   - 19-ID-E In-Situ Nanoprobe (ISN)
   - 26-ID-C CNM/APS Hard X-ray Nanoprobe (HXN)
   - 31-ID-E LYNX
-  - 33-ID-C PtychoProbe
   - 33-ID-C Velociprobe
   - 34-ID-C Microdiffraction, Coherent X-ray Scattering
 - Linac Coherent Light Source (LCLS)

src/ptychodus/api/plugins.py

@@ -116,6 +116,19 @@ def register_plugin(self, strategy: T, *, display_name: str, simple_name: str =
         if not simple_name:
             simple_name = re.sub(r'\W+', '', display_name)
 
+        # Settings persist the simple name, and lookup returns the first match, so a
+        # duplicate silently shadows the later registration and breaks its round trip.
+        # Warn rather than raise: load_plugins catches only AttributeError, and plugin
+        # loading must never be fatal.
+        for plugin in self._registered_plugins:
+            if plugin.simple_name.casefold() == simple_name.casefold():
+                logger.warning(
+                    f'Duplicate plugin simple name "{simple_name}": '
+                    f'"{display_name}" will be unreachable by that name '
+                    f'because "{plugin.display_name}" already claims it.'
+                )
+                break
+
         # The list is kept sorted by display name, so a registration can move the
         # selected plugin to a different index. Track it by identity across the sort
         # rather than by value: Plugin is a frozen dataclass, so equality compares the

src/ptychodus/plugins/fold_slice_product_file.py

@@ -21,7 +21,7 @@
 
 
 class FoldSliceProductFileReader(ProductFileReader):
-    SIMPLE_NAME: Final[str] = 'fold_slice'
+    SIMPLE_NAME: Final[str] = 'fold_slice_mat'
     DISPLAY_NAME: Final[str] = 'fold_slice Files (*.mat)'
 
     def read(self, file_path: Path) -> Product:

src/ptychodus/plugins/mda_position_file.py

@@ -528,7 +528,7 @@ def register_plugins(registry: PluginRegistry) -> None:
     registry.probe_position_file_readers.register_plugin(
         MDAFlatScanPositionFileReader(scale_to_meters=1.0e-6),
         simple_name='CNM_APS_HXN',
-        display_name='CNM/APS Hard X-ray Nanoprobe Files (*.mda)',
+        display_name='CNM/APS 26-ID Hard X-ray Nanoprobe Files (*.mda)',
     )
 
 

src/ptychodus/plugins/nsls2_diffraction_file.py

@@ -136,15 +136,15 @@ def register_plugins(registry: PluginRegistry) -> None:
     registry.diffraction_file_readers.register_plugin(
         NSLS2Style1DiffractionFileReader(),
         simple_name='NSLS_II_1',
-        display_name='NSLS-II Style 1 Files (*.h5 *.hdf5)',
+        display_name='NSLS-II 3-ID HXN Style 1 Files (*.h5 *.hdf5)',
     )
     registry.diffraction_file_readers.register_plugin(
         NSLS2Style2DiffractionFileReader(),
         simple_name='NSLS_II_2',
-        display_name='NSLS-II Style 2 Files (*.h5 *.hdf5)',
+        display_name='NSLS-II 3-ID HXN Style 2 Files (*.h5 *.hdf5)',
     )
     registry.diffraction_file_readers.register_plugin(
         NSLS2MATLABDiffractionFileReader(),
         simple_name='NSLS_II_MATLAB',
-        display_name='NSLS-II MATLAB Files (*.mat)',
+        display_name='NSLS-II 3-ID HXN MATLAB Files (*.mat)',
     )

src/ptychodus/plugins/nsls2_position_file.py

@@ -54,10 +54,10 @@ def register_plugins(registry: PluginRegistry) -> None:
     registry.probe_position_file_readers.register_plugin(
         NSLS2Style1PositionFileReader(),
         simple_name='NSLS_II_1',
-        display_name='NSLS-II Style 1 Files (*.h5 *.hdf5)',
+        display_name='NSLS-II 3-ID HXN Style 1 Files (*.h5 *.hdf5)',
     )
     registry.probe_position_file_readers.register_plugin(
         NSLS2Style2PositionFileReader(),
         simple_name='NSLS_II_2',
-        display_name='NSLS-II Style 2 Files (*.h5 *.hdf5)',
+        display_name='NSLS-II 3-ID HXN Style 2 Files (*.h5 *.hdf5)',
     )

src/ptychodus/plugins/nsls2_product_file.py

@@ -14,7 +14,7 @@
 
 class NSLSIIProductFileReader(ProductFileReader):
     SIMPLE_NAME: Final[str] = 'NSLS_II_MATLAB'
-    DISPLAY_NAME: Final[str] = 'NSLS-II MATLAB Files (*.mat)'
+    DISPLAY_NAME: Final[str] = 'NSLS-II 3-ID HXN MATLAB Files (*.mat)'
 
     def read(self, file_path: Path) -> Product:
         point_list: list[ProbePosition] = list()

src/ptychodus/plugins/tiff_diffraction_file.py

@@ -91,11 +91,22 @@ def read(self, file_path: Path) -> DiffractionDataset:
 
 
 def register_plugins(registry: PluginRegistry) -> None:
+    file_reader = TiffDiffractionFileReader()
     registry.diffraction_file_readers.register_plugin(
-        TiffDiffractionFileReader(),
+        file_reader,
         simple_name='TIFF',
         display_name='Tagged Image File Format Files (*.tif *.tiff)',
     )
+    registry.diffraction_file_readers.register_plugin(
+        file_reader,
+        simple_name='CNM_APS_HXN_TIFF',
+        display_name='CNM/APS 26-ID Hard X-ray Nanoprobe Files (*.tif *.tiff)',
+    )
+    registry.diffraction_file_readers.register_plugin(
+        file_reader,
+        simple_name='APS_34IDC',
+        display_name='APS 34-ID-C Microdiffraction Files (*.tif *.tiff)',
+    )
 
 
 if __name__ == '__main__':

tests/test_plugins.py

@@ -13,11 +13,14 @@
 
 from __future__ import annotations
 
+from collections import Counter
+import logging
+
 import pytest
 
 from ptychodus.api.observer import Observable, Observer
 from ptychodus.api.parameters import ParameterGroup, StringParameter
-from ptychodus.api.plugins import PluginChooser, PluginChooserParameter
+from ptychodus.api.plugins import PluginChooser, PluginChooserParameter, PluginRegistry
 
 
 class _Counter(Observer):
@@ -202,3 +205,60 @@ def test_two_adapters_over_one_chooser_both_track_it() -> None:
     assert parameter_b.get_value() == 'Zeta'
     assert settings_a.get_value() == 'Zeta'
     assert settings_b.get_value() == 'Zeta'
+
+
+def test_register_duplicate_simple_name_warns(caplog: pytest.LogCaptureFixture) -> None:
+    """A shadowed registration is announced rather than failing silently.
+
+    Settings persist the simple name and lookup returns the first match, so the
+    second registration under a taken name can never be selected from settings.
+    Registration still succeeds: plugin loading must never be fatal.
+    """
+    chooser: PluginChooser[str] = PluginChooser()
+    chooser.register_plugin('h5', display_name='Example Files (*.h5)', simple_name='example')
+
+    with caplog.at_level(logging.WARNING, logger='ptychodus.api.plugins'):
+        chooser.register_plugin('mat', display_name='Example Files (*.mat)', simple_name='example')
+
+    assert 'Example Files (*.h5)' in caplog.text
+    assert 'Example Files (*.mat)' in caplog.text
+
+    plugin = chooser.find_plugin('example')
+    assert plugin is not None
+    assert plugin.strategy == 'h5'
+
+
+def test_register_distinct_simple_names_is_quiet(caplog: pytest.LogCaptureFixture) -> None:
+    chooser: PluginChooser[str] = PluginChooser()
+    chooser.register_plugin('h5', display_name='Example Files (*.h5)', simple_name='example')
+
+    with caplog.at_level(logging.WARNING, logger='ptychodus.api.plugins'):
+        chooser.register_plugin(
+            'mat', display_name='Example Files (*.mat)', simple_name='example_mat'
+        )
+
+    assert 'Duplicate plugin simple name' not in caplog.text
+
+
+def test_load_plugins_has_no_duplicate_simple_names() -> None:
+    """Every chooser resolves each simple name to exactly one plugin.
+
+    A duplicate makes the later registration unreachable from settings, which is
+    how the two fold_slice probe-position readers once shadowed each other.
+    """
+    registry = PluginRegistry.load_plugins()
+    duplicates: list[str] = []
+
+    for attribute in dir(registry):
+        if attribute.startswith('_'):
+            continue
+
+        chooser = getattr(registry, attribute)
+
+        if not isinstance(chooser, PluginChooser):
+            continue
+
+        counter = Counter(plugin.simple_name.casefold() for plugin in chooser)
+        duplicates.extend(f'{attribute}: {name}' for name, count in counter.items() if count > 1)
+
+    assert not duplicates