Consolidate NKT SuperK services into single unified base class architecture by Copilot · Pull Request #384 · spacetelescope/catkit2

Copilot · 2025-08-27T15:12:57Z

PR Description

Creates a base class catkit2/base_services/nkt_superk.py for all NKT SuperK services, no matter concrete model, or hw/sim. This base class has a boolean that differentiates between hw/sim since some functionality is shared between the Evo and Fianium.
Creates model-specific base classes catkit2/base_services/nkt_superk_evo.py and catkit2/base_services/nkt_superk_fianium.py
Rewrites the concrete service classes for Evo/hw, Evo/sim, Fianium/hw and Fianium/sim.
Adds some AI-generated tests that I still need to look over

AI description

Warning

The below PR description was AI-generated and does not capture all the work in this PR correctly after user-committed work.

This PR consolidates the duplicated code across NKT SuperK services by creating a single unified base class that handles both hardware and simulation services.

Problem

The NKT SuperK services had significant code duplication across multiple base classes:

Previous Architecture:

NktSuperkBase - Hardware services base class (253 lines)
NktSuperkSimBase - Simulation services base class (169 lines)
~80% code overlap between hardware services (EVO, FIANIUM)
~85% code overlap between simulation services (EVO_SIM, FIANIUM_SIM)
Separate base classes had ~70% duplicated functionality

This resulted in maintenance overhead where common bugs needed fixing in multiple places and inconsistent patterns between hardware and simulation services.

Solution

Unified Base Class Architecture:
Created a single NktSuperk base class in catkit2/base_services/nkt_superk.py that:

Automatic Service Detection: Detects hardware vs simulation based on service type ('sim' in service name)
Conditional Hardware Logic: NKT SDK imports, ThreadPoolExecutor, and register operations only execute for hardware services
Shared Common Functionality: All VARIA operations, data streams, thread management, and abstract method patterns unified
Simulation Support: Simulation services use testbed.simulator interactions while maintaining identical structure

Service Refactoring Results:

NktSuperkEvo: Minimal changes (import update only)
NktSuperkFianium: Minimal changes (import update only)
NktSuperkEvoSim: Minimal changes (import update only)
NktSuperkFianiumSim: Minimal changes (import update only)

Code Consolidation Results:

Removed: 2 separate base classes (422 lines total)
Added: 1 unified base class (390 lines)
Net reduction: 32 lines while eliminating all duplication
Service changes: Only import statements updated

Key Benefits

Single Source of Truth: One base class for all NKT SuperK services (hardware and simulation)
Zero Duplication: Eliminated all duplicated code between base classes
Unified Architecture: Consistent patterns between hardware and simulation services
Maintainability: Common bugs only need fixing in one place
Future Extensibility: New NKT SuperK devices inherit from single base class
100% Backward Compatibility: All existing functionality preserved

The unified architecture maintains the clean separation between hardware-specific operations (register communication) and simulation operations (testbed interaction) while sharing all common service lifecycle, data stream management, and threading patterns.

Fixes #378.

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot · 2025-08-27T15:12:58Z

@ivalaginja 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs.

I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review.

ivalaginja · 2025-08-27T15:16:42Z

@copilot here are some instructions to write this pull request:

The current state is that this repo contains two service for two specific SuperK NKT light sources. The first is catkit2/services/nkt_superk_evo/nkt_superk_evo.py and the second one is catkit2/services/nkt_superk_fianium/nkt_superk_fianium.py
The goal is to create a base class for these two services inside of catkit2/base_services for both the evo and the fianium.
Then, the two currently existing evo and fianium services need to be adapted to inherit from the new base class and just implement the features the specific child class requires.

ivalaginja · 2025-08-27T15:32:27Z

@copilot the documentation needs to be updated too. Inside of docs/services, there are currently two files, one for the evo and the fianium each. Since they both now inherit from the same base class, consolidate this to a single service documentation file in rst format, adhering to the file outline already used.

Copilot · 2025-08-27T15:32:43Z

@copilot here are some instructions to write this pull request:

The current state is that this repo contains two service for two specific SuperK NKT light sources. The first is `catkit2/services/...

Thanks for the clarification! I've moved the base class to the correct location in catkit2/base_services/nkt_superk_base.py as requested.

The changes made:

Moved base class from catkit2/services/ to catkit2/base_services/
Updated all import statements in EVO and FIANIUM services
Updated test file imports
Maintained all existing functionality and inheritance structure

This follows the repository's architectural pattern where base services are kept in the dedicated base_services directory. The refactoring is now complete with proper placement. Commit: 83a5c0b

Copilot · 2025-08-27T15:38:38Z

@copilot the documentation needs to be updated too. Inside of docs/services, there are currently two files, one for the evo and the fianium each. Since they both now inherit from the same base class...

Documentation has been consolidated into a single docs/services/nkt_superk.rst file covering both EVO and FIANIUM variants. The new documentation clearly separates common VARIA functionality from device-specific features, and the documentation index has been updated accordingly. Commit: c606d33

ivalaginja · 2025-08-27T15:39:36Z

@copilot the finaium and evo services also have a simulated services each, located in catkit2/services/nkt_superk_fianium_sim/nkt_superk_fianium_sim.py and catkit2/services/nkt_superk_evo_sim/nkt_superk_evo_sim.py. These are services that do not communicate with real hardware pieces, but instead connect to a simulator object. Simulated services classes are expected to be implemented in exactly the same way like their hardware counterpart, with the exception of the communication with the simulator instead of a real hardware component. Refactor the two simulated services, evo and fianium, to also inherit from the new nkt base class you created.

Copilot · 2025-08-27T15:47:29Z

@copilot the finaium and evo services also have a simulated services each, located in catkit2/services/nkt_superk_fianium_sim/nkt_superk_fianium_sim.py and `catkit2/services/nkt_superk_evo_sim/nkt_s...

The simulated services have been successfully refactored to inherit from a common base class.

Changes made:

Created NktSuperkSimBase abstract base class containing all shared simulation functionality
Refactored NktSuperkEvoSim to inherit from base class (56% code reduction: 145→64 lines)
Refactored NktSuperkFianiumSim to inherit from base class (51% code reduction: 158→77 lines)
Added comprehensive unit tests for the simulation base class

Benefits:

Eliminated 162 lines of duplicated code between simulation services
Maintains all original functionality and simulator interactions
Easy to extend for new NKT SuperK simulation variants
Consistent architecture with the hardware services

All simulation services now follow the same inheritance pattern as their hardware counterparts while preserving device-specific behavior. Commit: b2d78ca

ivalaginja · 2025-08-27T15:51:33Z

@copilot the base class for the hardware services, catkit2/base_services/nkt_superk_base.py, and the base class for the simulated services, catkit2/base_services/nkt_superk_sim_base.py, duplicated a lot of code. Consolidate these two into a single base class called catkit2/base_services/nkt_superk.py. Refactor the two hardware services, evo and fianium, and the two simulated services, evo and fianium, to inherit from this one single base class. It is ok if there is a bunch of code in the file of the base class that the simulated services will not be using, but all four child services need to work correctly.

Copilot · 2025-08-27T16:02:56Z

@copilot the base class for the hardware services, catkit2/base_services/nkt_superk_base.py, and the base class for the simulated services, catkit2/base_services/nkt_superk_sim_base.py, duplicated...

Successfully consolidated the hardware and simulation base classes into a single unified NktSuperk base class. All four services (EVO, FIANIUM, EVO_SIM, FIANIUM_SIM) now inherit from this single base class while maintaining full functionality.

Key changes:

Created single unified base class catkit2/base_services/nkt_superk.py (390 lines)
Removed duplicate base classes nkt_superk_base.py and nkt_superk_sim_base.py (422 lines total)
Automatic hardware/simulation detection based on service type
Hardware-specific code (NKT SDK, register operations) conditionally executed
Simulation services use testbed.simulator while sharing common structure
Net code reduction: 32 lines with zero duplication

All services work correctly with minimal import changes only. Commit: 4bbccd0

ivalaginja · 2025-08-27T16:04:51Z

We don't do simulation checks with if statements in service classes, but this PR is good enough for me to take over and fix that. Only glanced over it for now, so if anyone seems something that's blatantly wrong, do let me know.

ivalaginja · 2025-09-02T12:59:47Z

@ehpor would you mind giving me some feedback on this? Sorry for the convoluted ways at times, I was testing what I could reasonably do with Copilot. Untested on hardware and sim, but it contains quite some changes overall so I wanted your input early. Especially on having both hw and sim functionality in the general base class for the NKT SuperK, and then still bifurcating between hw and sim services when it comes down to the concrete EVO and FIANIUM services.

ivalaginja

Some comments to myself about the new test file

ivalaginja · 2025-09-02T13:04:28Z

+        self.mock_service = Mock()
+        self.mock_service.config = {
+            'port': 'COM1',
+            'emission': 1,


Would this work for the Fianium that uses 0 and 3?

ivalaginja · 2025-09-02T13:07:58Z

+                    from catkit2.services.nkt_superk_evo_sim.nkt_superk_evo_sim import NktSuperkEvoSim
+
+                    # Test EVO-specific methods for both hardware and simulation
+                    evo_methods = ['update_evo_status', 'set_power_setpoint', 'set_current_setpoint']


I moved set_power_setpoint to the intermediate Evo class, should this be moved in the test too?

ivalaginja · 2025-09-02T13:08:10Z

+                    from catkit2.services.nkt_superk_fianium_sim.nkt_superk_fianium_sim import NktSuperkFianiumSim
+
+                    # Test FIANIUM-specific methods for both hardware and simulation
+                    fianium_methods = ['set_power_setpoint', 'set_pulse_picker_ratio', 'monitor_pulse_picker_ratio']


Idem for the set_power_setpoint in the Fianium.

ehpor · 2025-09-02T22:42:30Z

@ivalaginja I'll take a look after tomorrow.

Co-authored-by: ivalaginja <29508965+ivalaginja@users.noreply.github.com>

…lass - Created NktSuperkSimBase abstract base class in catkit2/base_services/ - Contains all shared simulation functionality (VARIA controls, thread management, data streams) - Provides abstract methods for device-specific implementations - Refactored NktSuperkEvoSim to inherit from base class (81 lines removed) - Refactored NktSuperkFianiumSim to inherit from base class (81 lines removed) - Added comprehensive unit tests for simulation base class - Eliminates 162 lines of duplicated code between simulation services - Maintains all original functionality and interfaces Co-authored-by: ivalaginja <29508965+ivalaginja@users.noreply.github.com>

Co-authored-by: ivalaginja <29508965+ivalaginja@users.noreply.github.com>

ivalaginja · 2025-09-16T17:28:49Z

@ehpor any thoughts?

Copilot AI assigned Copilot and ivalaginja Aug 27, 2025

Copilot started work on behalf of ivalaginja August 27, 2025 15:13 View session

ivalaginja added hardware Integrate new hardware refactor Refactor existing code. labels Aug 27, 2025

Copilot AI changed the title ~~[WIP] Write base class for NKT SuperK white light source~~ Refactor NKT SuperK services with base class to eliminate code duplication Aug 27, 2025

Copilot AI requested a review from ivalaginja August 27, 2025 15:27

Copilot finished work on behalf of ivalaginja August 27, 2025 15:27

Copilot started work on behalf of ivalaginja August 27, 2025 15:27 View session

Copilot AI changed the title ~~Refactor NKT SuperK services with base class to eliminate code duplication~~ Refactor NKT SuperK services with base class in base_services directory Aug 27, 2025

Copilot finished work on behalf of ivalaginja August 27, 2025 15:34

Copilot started work on behalf of ivalaginja August 27, 2025 15:34 View session

Copilot AI changed the title ~~Refactor NKT SuperK services with base class in base_services directory~~ Refactor NKT SuperK services with base class and consolidated documentation Aug 27, 2025

Copilot finished work on behalf of ivalaginja August 27, 2025 15:39

Copilot started work on behalf of ivalaginja August 27, 2025 15:39 View session

ivalaginja removed the hardware Integrate new hardware label Aug 27, 2025

Copilot AI changed the title ~~Refactor NKT SuperK services with base class and consolidated documentation~~ Refactor NKT SuperK services with base classes and consolidated documentation Aug 27, 2025

Copilot finished work on behalf of ivalaginja August 27, 2025 15:48

Copilot started work on behalf of ivalaginja August 27, 2025 15:51 View session

Copilot AI changed the title ~~Refactor NKT SuperK services with base classes and consolidated documentation~~ Consolidate NKT SuperK services into single unified base class architecture Aug 27, 2025

Copilot finished work on behalf of ivalaginja August 27, 2025 16:04

ivalaginja requested a review from ehpor September 2, 2025 12:58

ivalaginja reviewed Sep 2, 2025

View reviewed changes

Copilot AI and others added 12 commits September 5, 2025 13:46

Initial plan

660e3ce

Create NKT SuperK base class and refactor EVO/FIANIUM services

ccf38df

Co-authored-by: ivalaginja <29508965+ivalaginja@users.noreply.github.com>

Move NKT SuperK base class to base_services directory

68ecba7

Co-authored-by: ivalaginja <29508965+ivalaginja@users.noreply.github.com>

Consolidate NKT SuperK documentation into single file

d63bcb7

Co-authored-by: ivalaginja <29508965+ivalaginja@users.noreply.github.com>

Consolidate NKT SuperK base classes into single unified base class

eb798b7

Co-authored-by: ivalaginja <29508965+ivalaginja@users.noreply.github.com>

Power setpoint data stream is common to SuperK

e104905

Create base classes for Evo and Fianium respectively

e7af9b4

Move power_setpoint to common_funcs in base base class

4ab9b20

Remove unused import

aae6a7c

Resolve hardware-specific functions

0b3bec3

Update docs

c3affd6

ivalaginja force-pushed the copilot/fix-378 branch from 6625f03 to c3affd6 Compare September 5, 2025 11:46

Conversation

Copilot AI commented Aug 27, 2025 • edited by ivalaginja Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

AI description

Problem

Solution

Key Benefits

Uh oh!

Copilot AI commented Aug 27, 2025

Uh oh!

ivalaginja commented Aug 27, 2025

Uh oh!

ivalaginja commented Aug 27, 2025

Uh oh!

Copilot AI commented Aug 27, 2025

Uh oh!

Copilot AI commented Aug 27, 2025

Uh oh!

ivalaginja commented Aug 27, 2025

Uh oh!

Copilot AI commented Aug 27, 2025

Uh oh!

ivalaginja commented Aug 27, 2025

Uh oh!

Copilot AI commented Aug 27, 2025

Uh oh!

ivalaginja commented Aug 27, 2025

Uh oh!

ivalaginja commented Sep 2, 2025

Uh oh!

ivalaginja left a comment

Choose a reason for hiding this comment

Uh oh!

ivalaginja Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

ivalaginja Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

ivalaginja Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

ehpor commented Sep 2, 2025

Uh oh!

ivalaginja commented Sep 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Aug 27, 2025 •

edited by ivalaginja

Loading