Skip to content

[Sad Catapella N3][Scientific integrity] Parameter deserialization does not verify normalization_scope against requested settings #37

Closed

Description

@guijacquemet

Baseline and classification

Batch 3 at f784c95e3049483f87466150ffc5764375fa631d. Confirmed missing metadata check, but compatibility behavior is not prohibited by the current from_columns docstring (which only promises method validation); consider this a scientific-contract strengthening proposal rather than claiming a historical regression. No package tests executed.

Evidence

  • Parameters.from_columns lines 254–311 explicitly requires four metadata columns, validates that normalization_method matches settings.method_name, then returns the row's arbitrary normalization_scope and group. It does not compare normalization_scope to settings.scope or validate the scope against supported values.
  • NormalizationSettings.scope validation does refuse unsupported scopes at construction, but this check is bypassed for the deserialized parameter metadata.
  • Contract §4.1 assigns different scientific sampling populations to image, patch, and biological_repeat scopes. Fitting tests exercise the matching-scope case, not contradictory scopes.

Counterexample

Create row = Parameters('z-score','image','image-A',True,(10.0,),(2.0,)).to_columns(); then Parameters.from_columns(row, NormalizationSettings(method='zscore',scope='biological_repeat')) succeeds and returns image-scope parameters even though caller supplied biological-repeat settings. An unsupported text scope such as experiment also passes as a row scope.

Impact

Using fitted parameters for a scientifically different unit of analysis can silently change all normalized pixel values, with no exception at the deserialization boundary. Application itself is intentionally scope-agnostic, so the reader is the appropriate enforcement point if this contract is adopted.

Fix / decision

Validate normalization_scope == settings.scope by default and require an explicit separate override for intentional cross-scope reuse, if needed. Refuse unknown scopes with allowed choices. Consider an optional expected group/identity check at the calling-brick boundary rather than fabricating one here. Add matching/different scope, empty/unknown scope, changing group and per-time-point tests. Keep backward compatibility explicit. No Numba relevance.

Activity

  1. guijacquemet commented on Nov 3, 2026

    @guijacquemet
    ContributorAuthor

    This is still valid at f784c95: fitting.py:305 returns the row's normalization_scope without checking it.

    #62 fixes part of it. Parameters.from_columns now refuses a scope that is not biological_repeat, image or patch (for example experiment or a blank value). The tests are in tests/test_parameter_checks.py::test_from_columns_refuses_an_unknown_scope.

    The issue also asks for a strict normalization_scope == settings.scope check by default. I have not added it, because it would break a current consumer:

    • CellPatcher src/cellpatcher/normalization.py:270-276 (parameters_from_row, used by export.py:466 and delivery.py:183) builds NormalizationSettings without scope, so the default image is used, and then reads rows of every scope.
    • This repo's own tests/test_fitting.py::test_columns_round_trip_exactly reads patch rows with default settings.

    There are two ways to finish this; the owner needs to choose:

    1. Make the check strict by default and change CellPatcher to pass scope=settings.get("scope").
    2. Make it opt-in through a keyword.

    I'm leaving this issue open until that decision is made.


    Generated by Claude Code

  2. added a commit that references this issue on Nov 3, 2026
  3. guijacquemet commented on Nov 3, 2026

    @guijacquemet
    ContributorAuthor

    This is fixed by #63, merged in 3bca70d, and released in v0.3.3 (version bump #64, merge commit 320f304). The owner chose the strict option, so Parameters.from_columns now refuses a row whose normalization_scope differs from settings.scope. It still refuses unknown scopes, as before.

    This does not affect CellPatcher until it repins past v0.3.2. When it repins, parameters_from_row must pass scope=settings.get("scope", "image") to NormalizationSettings. Rows from a removed scope (condition, technical_repeat) have been refused as unknown scopes since #62. CellPatcher has to refuse those rows or build Parameters from the pair columns itself, as it already does for legacy ; rows. The details are in #63.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions