major update: add channel configuration file management #100

Alpaca233 · 2025-02-10T22:40:01Z

Tested in simulation mode
New file structure:
software/acquisition_configurations/default_configurations/4x/
configuration files:
channel_configurations.xml
confocal_configurations.xml (optional)
widefield_configuration.xml (optional)
laser_af_reference_plane.json (optional)

hongquanli · 2025-02-10T23:34:20Z

software/acquisition_configurations/default_configurations/60x/laser_af_reference_plane.json

I think these should not be on GitHub. We can have code that will generate these (maybe this is already generated by code)?

hongquanli · 2025-02-10T23:42:11Z

software/control/utils.py

+def get_channel_color(self, channel):
+    channel_info = CHANNEL_COLORS_MAP.get(extract_wavelength_from_config_name(channel), {"hex": 0xFFFFFF, "name": "gray"})
+    return channel_info["hex"]


Will all channels have gray color?

Not very sure about this. It was in the old ConfigurationManager and I moved it to utils because at a few places we are passing the whole ConfigurationManager only to use get_channel_color and extract_wavelength

hongquanli · 2025-02-10T23:46:39Z

software/control/gui_hcs.py

-                use_glass_top=USE_GLASS_TOP,
-                look_for_cache=False,
+                self.objectiveStore,
+                self.acquisitionConfigurationManager.get_autofocus_configurations()


(mark this to look at - laserAutofocusController probably should not access self.acquisitionConfigurationManager)

…rom ACM class

ianohara

I left a bunch of comments. Some other observations:

It looks like the new ChannelConfigurationManager is only ever used when there's a ConfigurationManager present. Instead of needing to pass around both, would it make sense for us to just put a AcquisitionConfigurations or dict[ConfigTypeFlag, List[Configurations]] attribute on the ConfigurationManager, and then just add a few helpers to that object for getting configs?
There are a few hard things about the codebase, but one of the hardest is the fact that we store related state in a bunch of separate places, and in some cases store the same state in multiple places. My suggestion is to get rid of all the active_* attributes, and just store the ConfigTypeFlag and current Configuration in one place. Preferably together in a way that requires updating both if you're going to update 1.
Make all the xml read/write functions build the xml at the time of writing/parsing and eliminate all the intermediate xml tree storage. Just use xml as our serialization format.

ianohara · 2025-02-13T20:37:50Z

software/control/core/core.py

-        for mode in self.config_xml_tree_root.iter("mode"):
-            self.num_configurations += 1
-            self.configurations.append(
+class ChannelConfigurationManager:


Maybe we considered this already and it doesn't make sense, but there's a pydantic <-> xml library ( https://pydantic-xml.readthedocs.io/en/latest/ ) that would let us use pydantic models for configuration, and handles writing / reading them from xml files for us.

This would be nice because:

We'd get all the benefits of using pydantic models (IDEs understand them, some types, validation of values, consistency in our codebase)

We don't need to manually serialize/deserialize the xml structure in code. This is really fragile and likely to break as we make changes!

Looks nice. Will try this out

@Alpaca233 can we get rid of "CHANNEL" config type?

ianohara · 2025-02-13T20:41:12Z

software/control/core/core.py

+    def set_profile_path(self, profile_path: Path) -> None:
+        self.current_profile_path = profile_path
+
+    def load_configurations(self, objective: str) -> None:


I'm not sure of the right answer (or if there is one), but what led us to a directory structure + multiple xml files instead of 1 xml file containing all the configurations? The single file approach seems simpler (we only need to know 1 path), and easier to distribute (we distribute a single file, not a directory structure with many files). I guess one down side (maybe?) is that it could be a pretty big file, and picking and choosing only individual configurations might be trickier. But we're already requiring someone to modify xml if they aren't using our UI, so I'm not sure it's worse (if anything, they only need to deal with xml instead of a directory structure and files that need correct names).

So in each objective folder there could be multiple files. Now we have channel configuration file (xml) and autofocus cache file (json). I guess this would be easier to find and make changes to them. Also may be easier to add an objective and another type of configuration file? This is @hongquanli 's idea so he may have a better answer.

I don't have strong objective thoughts either way. My comment comes from a place of trying to simplify (it is simpler if we only need to keep track of a single datastructure (or a few), versus a few datastructures, a directory format, and a file naming convention).

Both can definitely work, though. And if we want heterogeneous serialization formats for some reason, then it definitely makes sense.

ianohara · 2025-02-13T20:43:37Z

software/control/core/core.py

+        objective_path = self.current_profile_path / objective
+
+        # Load spinning disk configurations if enabled
+        if ENABLE_SPINNING_DISK_CONFOCAL:


Whenever possible, if we can de-couple from global configuration that'd be helpful. In this case, we could just check to see if confocal_conifgurations.xml exists. If it does, load it. If it doesn't, don't (same for the widefield_ and channel_ files). Then anywhere that needs that configuration later on can check if the ChannelConfigurationManager it is using has it or not (they'll already need to check if ENABLE_SPINNING_DISK_CONFOCAL is True there anyway).

Will change this

ianohara · 2025-02-13T20:46:14Z

software/control/core/core.py

-        self.save_configurations()
+    def save_configurations(self, objective: str) -> None:
+        """Save channel configurations for a specific objective."""
+        if ENABLE_SPINNING_DISK_CONFOCAL:


Same here (and everywhere we check for ENABLE_SPINNING_DISK_CONFOCAL in this class). Is there a disadvantage to just checking what config is active and writing that (regardless of the ENABLE_* check)?

ianohara · 2025-02-13T20:52:20Z

software/control/core/core.py

+        self.active_channel_config = None
+        self.active_config_xml_tree = None
+        self.active_config_xml_tree_root = None
+        self.active_config_flag = -1  # 0: channel, 1: confocal, 2: widefield


It'd be better to use an enum.Enum class for this. In other words, something like this:

class ConfigTypeFlag(enum.Enum): UNKNOWN = auto() CHANNEL = auto() CONFOCAL = auto() WIDEFIELD = auto()

Then

self.active_config_flag = ConfigTypeFlag.UNKNOWN

This is better for a couple of reasons:

We don't need to remember numbers, and don't use bare numbers elsewhere in code. The later is particularly important because if we use bare numbers (eg: 1), it's impossible to distinguish between different occurrences of 1 in our codebase without understanding all the context it is used it (does that 1 mean active_config_flag, or a quantity 1 of something, or some other flag?).

IDEs and tools know how to handle the enum, so if we ever need to refactor it's really easy.

Will change this

While reviewing Hongquan's #100, we can across the fact that auto() is a little dangerous. So instead I'd use:

class ConfigTypeFlag(enum.Enum): UNKNOWN = "UNKNOWN" CHANNEL = "CHANNEL" CONFOCAL = "CONFOCAL" WIDEFIELD = "WIDEFIELD"

for now.

ianohara · 2025-02-13T22:06:51Z

software/control/utils.py

@@ -165,3 +167,21 @@ def ensure_directory_exists(raw_string_path: str):
    path: pathlib.Path = pathlib.Path(raw_string_path)
    _log.debug(f"Making sure directory '{path}' exists.")
    path.mkdir(parents=True, exist_ok=True)
+
+
+def extract_wavelength_from_config_name(name):


This seems pretty fragile! If we depend on the wavelength for config, should it instead be a config attribute? It's a bit funky to store some of our config values in the object itself, and some in the name.

Indeed. I think there were a few other cases where we need to extract an attribute from the name. We may need to update this config object

ianohara · 2025-02-13T22:07:27Z

software/control/widgets.py

@@ -66,7 +66,8 @@ def __init__(self, title):
    def toggle_content(self, state):
        self.content_widget.setVisible(state)

-
+'''


Looks like a bunch of things are marked like this. I won't comment on them from now on, but make sure to delete or fix before merging!

ianohara · 2025-02-13T22:08:02Z

software/control/widgets.py

@@ -329,6 +330,71 @@ def apply_and_exit(self):
        self.close()


+class LaserAutofocusSettingsWidget(QDialog):
+    def __init__(self, laser_af_controller):


It'd be nice to add the type annotation on laser_af_controller here.

Will change this

ianohara · 2025-02-13T22:12:07Z

software/control/widgets.py

@@ -347,17 +413,12 @@ def __init__(self, xlight, config_manager=None):

        self.disk_position_state = self.xlight.get_disk_position()

+        if self.config_manager is not None:


Normally for object presence checks, you can just do:

if self.config_manager: # do things with config manager

What you've done works too, just the direct check is a bit less typing. It only breaks down if you have an object that modifies its __bool__ method or len method (see https://docs.python.org/3/reference/datamodel.html#object.__bool__ ). But it's odd for an object to do that if it's not a math/collection type of some sort.

Will change this

ianohara · 2025-02-13T22:13:57Z

software/control/widgets.py

-        self.currentConfiguration = self.configurationManager.configurations[0]
-
+        if self.configurationManager and self.channelConfigurationManager:
+            self.currentConfiguration = self.channelConfigurationManager.get_channel_configurations_for_objective(self.objectiveStore.current_objective)[0]


Hmm it looks like we store our currentConfiguration here. It's tricky to follow if we store the active config here, but also have an active channel stored on the channelConfigurationManager, and the active objective elsewhere. Ideally, we'd just store the active configuration information in one place and use that to query information from other sources as needed.

Also, is this [0] always guaranteed safe?

hongquanli · 2025-02-22T22:52:04Z

software/control/core/core.py

+            self.autofocus_configurations[objective] = {}
+        self.autofocus_configurations[objective].update(updates)
+
+class ConfigurationManager(QObject):


Looks like it does not need to be a QObject

hongquanli · 2025-02-22T23:25:59Z

software/control/core/core.py

+    def _get_available_profiles(self) -> List[str]:
+        if not self.base_config_path.exists():
+            os.makedirs(self.base_config_path)
+            os.makedirs(self.base_config_path / "default_profile")
+            for objective in OBJECTIVES:
+                os.makedirs(self.base_config_path / "default_profile" / objective)
+        return [d.name for d in self.base_config_path.iterdir() if d.is_dir()]


This needs more documentations. What are the "available profiles"?

hongquanli · 2025-02-22T23:30:00Z

software/control/core/core.py

@@ -4433,7 +4560,6 @@ def get_surface_grid(self, x_range, y_range, num_points=50):


 class LaserAutofocusController(QObject):


We should have a separate PR to update LaserAutofocusController (including make it use laserAFConfig base model)

For this PR we can leave this class unchanged (revert the modifications). It would have been better to update LaserAutofocusController to make it use laserAFConfig before doing this PR.

hongquanli · 2025-02-23T01:08:39Z

software/control/core/core.py

+        self.color = utils.get_channel_color(self.name)
+
+
+QMetaType.registerType(ChannelMode)


What does this do?

hongquanli · 2025-02-23T01:11:09Z

software/control/core/core.py

+class ChannelConfig(BaseXmlModel, tag='configurations'):
+    """Root configuration file model"""
+    modes: List[ChannelMode] = element(tag='mode')


Add more description on what this is and how this is used?

hongquanli · 2025-02-23T04:52:13Z

software/control/core/core.py

@@ -3690,7 +3690,8 @@ def load_configurations(self, objective: str) -> None:
        config_file = self.current_profile_path / objective / "laser_af_cache.json"


we should rename laser af cache to laser af setting

hongquanli · 2025-02-23T04:55:05Z

software/control/core/core.py

    def update_laser_af_cache(self, objective: str, updates: Dict[str, Any]) -> None:
        if objective not in self.autofocus_configurations:
-            self.autofocus_configurations[objective] = {}
-        self.autofocus_configurations[objective].update(updates)
+            self.autofocus_configurations[objective] = LaserAFConfig(**updates)
+        else:
+            config = self.autofocus_configurations[objective]
+            self.autofocus_configurations[objective] = config.model_copy(update=updates)


Why do the two cases need to be treated separately? Can we use self.autofocus_configurations[objective] = LaserAFConfig(**updates) for both?

LaserAFConfig(**updates) creates a new BaseModel object with default values if the laser af cache doesn't exist, and update the items as needed. model_copy update the current model loaded in memory. If we use LaserAFConfig(**updates) we will lose the information of the loaded model.

Alpaca233 added 7 commits February 9, 2025 23:36

AcquisitionConfigurationManager class and LaserAutofocusController class

e32c8da

replaced old usages

be36177

channel configuration for confocal

22955e9

active channel flag; update write_configuration_selected

1cc16cf

example configuration files

76bdadb

fix json

46e6ce3

ui components for configuration profile management

19cc63c

hongquanli reviewed Feb 10, 2025

View reviewed changes

bug fix

c6fbba0

hongquanli reviewed Feb 10, 2025

View reviewed changes

Alpaca233 added 5 commits February 11, 2025 16:06

extract ChannelConfigurationManager and LaserAFConfigurationManager f…

92f505f

…rom ACM class

update channel configuration and laser af cache usages everywhere

f77a27a

gui laser af setting

b175456

bug fix

735fd47

bug fix

a6b2593

ianohara reviewed Feb 13, 2025

View reviewed changes

Alpaca233 added 5 commits February 14, 2025 22:33

new laser af configuration widget

f9f3a62

use pydantic-xml

ef49349

update usages

e85068c

addressed comments

513f84d

bug fix; create default profile with pydantic-xml

15bbc0e

hongquanli reviewed Feb 22, 2025

View reviewed changes

hongquanli reviewed Feb 23, 2025

View reviewed changes

Alpaca233 added 3 commits February 22, 2025 17:55

use pydantic for laser af cache

7e51a52

ProfileWidget

d5fe609

bug fix: block signals when adding items to channel mode dropdown

df06321

hongquanli reviewed Feb 23, 2025

View reviewed changes

Alpaca233 added 3 commits February 22, 2025 21:14

updated naming

413fd59

updated naming

b16ee0c

documentation

c146bae

hongquanli marked this pull request as ready for review February 23, 2025 05:43

Merge branch 'master' into laser_af_cache

2aabf99

hongquanli changed the title ~~Updated channel acquisition configuration file and laser af cache file management~~ major update: add channel configuration file management Feb 23, 2025

hongquanli merged commit 35e77b9 into Cephla-Lab:master Feb 23, 2025
0 of 2 checks passed

		@@ -347,17 +413,12 @@ def __init__(self, xlight, config_manager=None):

		self.disk_position_state = self.xlight.get_disk_position()

		if self.config_manager is not None:

		@@ -4433,7 +4560,6 @@ def get_surface_grid(self, x_range, y_range, num_points=50):


		class LaserAutofocusController(QObject):

		self.color = utils.get_channel_color(self.name)


		QMetaType.registerType(ChannelMode)

		@@ -3690,7 +3690,8 @@ def load_configurations(self, objective: str) -> None:
		config_file = self.current_profile_path / objective / "laser_af_cache.json"

major update: add channel configuration file management #100

major update: add channel configuration file management #100

Conversation

Alpaca233 commented Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianohara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongquanli Feb 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alpaca233 commented Feb 10, 2025 •

edited

Loading

hongquanli Feb 22, 2025 •

edited

Loading