Skip to content

Simplify logical view API with instrument.add_logical_view() #647

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
SimonHeybrock merged 4 commits into from
Jan 8, 2026
Merged

Simplify logical view API with instrument.add_logical_view() #647

SimonHeybrock merged 4 commits into from
Jan 8, 2026

Conversation

@SimonHeybrock
Copy link
Member

@SimonHeybrock SimonHeybrock commented Jan 8, 2026 •
edited
Loading

Summary

  • Introduces instrument.add_logical_view() to couple spec registration with transform function
  • Removes the error-prone two-phase pattern (spec handle in specs.py + manual factory attachment in factories.py)
  • Converts all instruments (DREAM, ESTIA, TBL, ODIN) to the new pattern
  • Removes register_logical_detector_view_spec() which is now redundant
  • Establishes views.py convention for transform functions

Motivation

The previous pattern for logical detector views was error-prone:

# specs.py - register spec, get handle
view_handle = register_logical_detector_view_spec(
    instrument=instrument,
    name='detector_view',
    ...
)

# factories.py - create DetectorLogicalView, attach to handle
_view = DetectorLogicalView(
    instrument=instrument,
    transform=lambda da: da.fold(...),
    reduction_dim=['x_bin', 'y_bin'],
)
specs.view_handle.attach_factory()(_view.make_view)

Problems:

  • Decoupled: Spec metadata in specs.py, transform function in factories.py
  • Error-prone: Easy to forget to attach factory, or attach wrong factory to wrong handle
  • Verbose: Boilerplate for every logical view

The new add_logical_view() couples everything together:

# specs.py - one call does it all
instrument.add_logical_view(
    name='detector_view',
    title='Detector View',
    description='...',
    source_names=['detector1'],
    transform=get_detector_view,  # from views.py
    reduction_dim=['x_bin', 'y_bin'],
)

The factory is auto-attached during load_factories(). Parameters map directly to DetectorLogicalView, so if you know one API, you know the other.

Test plan

  • All 1095 existing tests pass
  • New tests in logical_view_test.py for add_logical_view() and LogicalViewConfig
  • All instrument workflows verified via registered_workflow_factories_test.py
  • Documentation updated in docs/user-guide/adding-new-instruments.md

🤖 Generated with Claude Code

SimonHeybrock and others added 4 commits January 8, 2026 08:31
@SimonHeybrock @claude
Add LogicalViewRegistry for coupled transform-spec registration
03bb4e1 
Introduce a registry pattern that couples transform functions with their
spec metadata, preventing the error-prone pattern of separately defining
handles in specs.py and transforms in factories.py.

Key changes:
- Add LogicalViewRegistry class in handlers/logical_view_registry.py
- Add LogicalViewConfig dataclass for view configuration
- Migrate DREAM's mantle logical views to use the registry pattern:
  - Create views.py with transform functions and registry
  - Simplify specs.py to call mantle_views.register_specs()
  - Simplify factories.py to call mantle_views.attach_factories()

Benefits:
- Transform and spec metadata are coupled in one place
- Two-phase registration is preserved (lightweight specs, heavy factories)
- Type safety and IDE support (no string-based coupling)
- Clear pattern for adding new views: define transform, add to registry
- Reduces boilerplate when adding multiple similar views

The registry accepts the two-phase registration pattern rather than
fighting it, making the coupling explicit while keeping specs.py
lightweight. This scales better for 20+ instruments over many years.

Context: Analyzed repetition in DREAM's mantle_wire_view_handle and
mantle_strip_view_handle registrations. We explored lazy_import to
eliminate two-phase registration entirely, but concluded that the
LogicalViewRegistry approach is more maintainable at scale because it
preserves type safety and IDE support while making the transform-spec
coupling explicit.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@SimonHeybrock
Cleanup
5417e8d
@SimonHeybrock @claude
Replace LogicalViewRegistry with instrument.add_logical_view()
5f8fa93 
Simplify logical view registration by moving it directly into the Instrument
class. This eliminates the need for a separate registry abstraction and reduces
the boilerplate of importing and calling methods in multiple places.

- Add LogicalViewConfig dataclass and add_logical_view() method to Instrument
- Update load_factories() to automatically attach logical view factories
- Update DREAM instrument to use the new API
- Remove LogicalViewRegistry module

Prompt: Please have a look at the changes in this branch, introducing
LogicalViewRegistry. Note how we have to import the instance in both specs.py
and factories.py and call a specific method. I wonder if this could be avoided
by simply allowing to "add" registries to Instrument, which could then call the
appropriate methods in the right place?

Follow-up: User preferred instrument.add_logical_view() over registry pattern.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@SimonHeybrock @claude
Add reduction_dim to add_logical_view and inline spec registration
533d546 
Extend add_logical_view() to accept reduction_dim parameter, aligning its
API with DetectorLogicalView. This simplifies the mental model: parameters
to add_logical_view map directly to DetectorLogicalView constructor args.

Convert all instruments using register_logical_detector_view_spec to use
add_logical_view instead, then inline the function since it has no direct
callers. This reduces API surface from two ways of registering logical
views to one.

Changes:
- Add reduction_dim to LogicalViewConfig and add_logical_view
- Inline register_logical_detector_view_spec logic into add_logical_view
- Convert ESTIA, TBL, ODIN to use add_logical_view with transforms in
  new views.py files (following DREAM's pattern)
- Remove manual factory attachments from factories.py files
- Remove register_logical_detector_view_spec function and its tests

Prompt: "Please think about whether you would recommend this [inlining
register_logical_detector_view_spec into add_logical_view]. Would it make
the overall API surface simpler and cleaner?"

Follow-up: "But don't we already have the same complexity present in
DetectorLogicalView? Wouldn't making add_logical_view which conceptually
'creates a DetectorLogicalView' have the same arguments be easier to
remember/understand?"

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@SimonHeybrock SimonHeybrock merged commit 8d048fc into main Jan 8, 2026
4 checks passed
@SimonHeybrock SimonHeybrock deleted the logical-view-registry branch January 8, 2026 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SimonHeybrock