diff --git a/src/easyscience/global_object/map.py b/src/easyscience/global_object/map.py index 53a3aac4..0f29c1bc 100644 --- a/src/easyscience/global_object/map.py +++ b/src/easyscience/global_object/map.py @@ -261,7 +261,7 @@ def is_connected(self, vertices_encountered=None, start_vertex=None) -> bool: return False def _clear(self): - """Reset the map to an empty state.""" + """Reset the map to an empty state. Only to be used for testing""" for vertex in self.vertices(): self.prune(vertex) gc.collect() diff --git a/src/easyscience/variable/descriptor_number.py b/src/easyscience/variable/descriptor_number.py index 66f0e7ab..b798caa4 100644 --- a/src/easyscience/variable/descriptor_number.py +++ b/src/easyscience/variable/descriptor_number.py @@ -1,6 +1,7 @@ from __future__ import annotations import numbers +import uuid from typing import Any from typing import Dict from typing import List @@ -50,6 +51,7 @@ def __init__( url: Optional[str] = None, display_name: Optional[str] = None, parent: Optional[Any] = None, + **kwargs: Any # Additional keyword arguments (used for (de)serialization) ): """Constructor for the DescriptorNumber class @@ -65,6 +67,10 @@ def __init__( """ self._observers: List[DescriptorNumber] = [] + # Extract dependency_id if provided during deserialization + if '__dependency_id' in kwargs: + self.__dependency_id = kwargs.pop('__dependency_id') + if not isinstance(value, numbers.Number) or isinstance(value, bool): raise TypeError(f'{value=} must be a number') if variance is not None: @@ -112,10 +118,14 @@ def from_scipp(cls, name: str, full_value: Variable, **kwargs) -> DescriptorNumb def _attach_observer(self, observer: DescriptorNumber) -> None: """Attach an observer to the descriptor.""" self._observers.append(observer) + if not hasattr(self, '_DescriptorNumber__dependency_id'): + self.__dependency_id = str(uuid.uuid4()) def _detach_observer(self, observer: DescriptorNumber) -> None: """Detach an observer from the descriptor.""" self._observers.remove(observer) + if not self._observers: + del self.__dependency_id def _notify_observers(self) -> None: """Notify all observers of a change.""" @@ -323,6 +333,8 @@ def as_dict(self, skip: Optional[List[str]] = None) -> Dict[str, Any]: raw_dict['value'] = self._scalar.value raw_dict['unit'] = str(self._scalar.unit) raw_dict['variance'] = self._scalar.variance + if hasattr(self, '_DescriptorNumber__dependency_id'): + raw_dict['__dependency_id'] = self.__dependency_id return raw_dict def __add__(self, other: Union[DescriptorNumber, numbers.Number]) -> DescriptorNumber: diff --git a/src/easyscience/variable/parameter.py b/src/easyscience/variable/parameter.py index 39692963..3714ba7b 100644 --- a/src/easyscience/variable/parameter.py +++ b/src/easyscience/variable/parameter.py @@ -11,6 +11,7 @@ import weakref from typing import Any from typing import Dict +from typing import List from typing import Optional from typing import Union @@ -36,14 +37,6 @@ class Parameter(DescriptorNumber): # We copy the parent's _REDIRECT and modify it to avoid altering the parent's class dict _REDIRECT = DescriptorNumber._REDIRECT.copy() _REDIRECT['callback'] = None - # Skip these attributes during normal serialization as they are handled specially - _REDIRECT['_dependency_interpreter'] = None - _REDIRECT['_clean_dependency_string'] = None - # Skip the new serialization parameters - they'll be handled by _convert_to_dict - _REDIRECT['_dependency_string'] = None - _REDIRECT['_dependency_map_unique_names'] = None - _REDIRECT['_dependency_map_dependency_ids'] = None - _REDIRECT['__dependency_id'] = None def __init__( self, @@ -84,11 +77,8 @@ def __init__( """ # noqa: E501 # Extract and ignore serialization-specific fields from kwargs kwargs.pop('_dependency_string', None) - kwargs.pop('_dependency_map_unique_names', None) kwargs.pop('_dependency_map_dependency_ids', None) kwargs.pop('_independent', None) - # Extract dependency_id if provided during deserialization - provided_dependency_id = kwargs.pop('__dependency_id', None) if not isinstance(min, numbers.Number): raise TypeError('`min` must be a number') @@ -119,6 +109,7 @@ def __init__( url=url, display_name=display_name, parent=parent, + **kwargs, # Additional keyword arguments (used for (de)serialization) ) self._callback = callback # Callback is used by interface to link to model @@ -128,13 +119,6 @@ def __init__( # Create additional fitting elements self._initial_scalar = copy.deepcopy(self._scalar) - # Generate unique dependency ID for serialization/deserialization - # Use provided dependency_id if available (during deserialization), otherwise generate new one - if provided_dependency_id is not None: - self.__dependency_id = provided_dependency_id - else: - import uuid - self.__dependency_id = str(uuid.uuid4()) @classmethod def from_dependency(cls, name: str, dependency_expression: str, dependency_map: Optional[dict] = None, **kwargs) -> Parameter: # noqa: E501 @@ -147,7 +131,7 @@ def from_dependency(cls, name: str, dependency_expression: str, dependency_map: :param kwargs: Additional keyword arguments to pass to the Parameter constructor. :return: A new dependent Parameter object. """ # noqa: E501 - # Set default values for required parameters if not provided in kwargs + # Set default values for required parameters for the constructor, they get overwritten by the dependency anyways default_kwargs = { 'value': 0.0, 'unit': '', @@ -155,7 +139,7 @@ def from_dependency(cls, name: str, dependency_expression: str, dependency_map: 'min': -np.inf, 'max': np.inf } - # Update with user-provided kwargs, giving precedence to user values + # Update with user-provided kwargs, to avoid errors. default_kwargs.update(kwargs) parameter = cls(name=name, **default_kwargs) parameter.make_dependent_on(dependency_expression=dependency_expression, dependency_map=dependency_map) @@ -331,15 +315,6 @@ def dependency_map(self) -> Dict[str, DescriptorNumber]: def dependency_map(self, new_map: Dict[str, DescriptorNumber]) -> None: raise AttributeError('Dependency map is read-only. Use `make_dependent_on` to change the dependency map.') - @property - def dependency_id(self) -> str: - """ - Get the unique dependency ID of this parameter used for serialization. - - :return: The dependency ID of this parameter. - """ - return self.__dependency_id - @property def value_no_call_back(self) -> numbers.Number: """ @@ -553,6 +528,26 @@ def free(self) -> bool: def free(self, value: bool) -> None: self.fixed = not value + def as_dict(self, skip: Optional[List[str]] = None) -> Dict[str, Any]: + """ Overwrite the as_dict method to handle dependency information. """ + raw_dict = super().as_dict(skip=skip) + + + # Add dependency information for dependent parameters + if not self._independent: + # Save the dependency expression + raw_dict['_dependency_string'] = self._clean_dependency_string + + # Mark that this parameter is dependent + raw_dict['_independent'] = self._independent + + # Convert dependency_map to use dependency_ids + raw_dict['_dependency_map_dependency_ids'] = {} + for key, obj in self._dependency_map.items(): + raw_dict['_dependency_map_dependency_ids'][key] = obj._DescriptorNumber__dependency_id + + return raw_dict + def _revert_dependency(self, skip_detach=False) -> None: """ Revert the dependency to the old dependency. This is used when an error is raised during setting the dependency. @@ -595,63 +590,26 @@ def _process_dependency_unique_names(self, dependency_expression: str): raise ValueError(f'The object with unique_name {stripped_name} is not a Parameter or DescriptorNumber. Please check your dependency expression.') # noqa: E501 self._clean_dependency_string = clean_dependency_string - def _convert_to_dict(self, d: dict, encoder, skip=None, **kwargs) -> dict: - """Custom serialization to handle parameter dependencies.""" - if skip is None: - skip = [] - - # Add dependency information for dependent parameters - if not self._independent: - # Save the dependency expression - d['_dependency_string'] = self._dependency_string - - # Convert dependency_map to use dependency_ids (preferred) and unique_names (fallback) - d['_dependency_map_dependency_ids'] = {} - d['_dependency_map_unique_names'] = {} - - for key, dep_obj in self._dependency_map.items(): - # Store dependency_id if available (more reliable) - if hasattr(dep_obj, '__dependency_id'): - d['_dependency_map_dependency_ids'][key] = dep_obj.__dependency_id - # Also store unique_name as fallback - if hasattr(dep_obj, 'unique_name'): - d['_dependency_map_unique_names'][key] = dep_obj.unique_name - else: - # This is quite impossible - throw an error - raise ValueError(f'The object with unique_name {key} does not have a unique_name attribute.') - - # Always include dependency_id for this parameter - d['__dependency_id'] = self.__dependency_id - - # Mark that this parameter is dependent - d['_independent'] = self._independent - - return d - - @classmethod + @classmethod def from_dict(cls, obj_dict: dict) -> 'Parameter': """ Custom deserialization to handle parameter dependencies. Override the parent method to handle dependency information. """ # Extract dependency information before creating the parameter - d = obj_dict.copy() # Don't modify the original dict - dependency_string = d.pop('_dependency_string', None) - dependency_map_unique_names = d.pop('_dependency_map_unique_names', None) - dependency_map_dependency_ids = d.pop('_dependency_map_dependency_ids', None) - is_independent = d.pop('_independent', True) - # Note: Keep __dependency_id in the dict so it gets passed to __init__ + raw_dict = obj_dict.copy() # Don't modify the original dict + dependency_string = raw_dict.pop('_dependency_string', None) + dependency_map_dependency_ids = raw_dict.pop('_dependency_map_dependency_ids', None) + is_independent = raw_dict.pop('_independent', True) + # Note: Keep _dependency_id in the dict so it gets passed to __init__ # Create the parameter using the base class method (dependency_id is now handled in __init__) - param = super().from_dict(d) + param = super().from_dict(raw_dict) # Store dependency information for later resolution - if not is_independent and dependency_string is not None: + if not is_independent: param._pending_dependency_string = dependency_string - if dependency_map_dependency_ids: - param._pending_dependency_map_dependency_ids = dependency_map_dependency_ids - if dependency_map_unique_names: - param._pending_dependency_map_unique_names = dependency_map_unique_names + param._pending_dependency_map_dependency_ids = dependency_map_dependency_ids # Keep parameter as independent initially - will be made dependent after all objects are loaded param._independent = True @@ -995,13 +953,12 @@ def resolve_pending_dependencies(self) -> None: """Resolve pending dependencies after deserialization. This method should be called after all parameters have been deserialized - to establish dependency relationships using dependency_ids or unique_names as fallback. + to establish dependency relationships using dependency_ids. """ if hasattr(self, '_pending_dependency_string'): dependency_string = self._pending_dependency_string dependency_map = {} - # Try dependency IDs first (more reliable) if hasattr(self, '_pending_dependency_map_dependency_ids'): dependency_map_dependency_ids = self._pending_dependency_map_dependency_ids @@ -1013,22 +970,6 @@ def resolve_pending_dependencies(self) -> None: else: raise ValueError(f"Cannot find parameter with dependency_id '{dependency_id}'") - # Fallback to unique_names if dependency IDs not available or incomplete - if hasattr(self, '_pending_dependency_map_unique_names'): - dependency_map_unique_names = self._pending_dependency_map_unique_names - - for key, unique_name in dependency_map_unique_names.items(): - if key not in dependency_map: # Only add if not already resolved by dependency_id - try: - # Look up the parameter by unique_name in the global map - dep_obj = self._global_object.map.get_item_by_key(unique_name) - if dep_obj is not None: - dependency_map[key] = dep_obj - else: - raise ValueError(f"Cannot find parameter with unique_name '{unique_name}'") - except Exception as e: - raise ValueError(f"Error resolving dependency '{key}' -> '{unique_name}': {e}") - # Establish the dependency relationship try: self.make_dependent_on(dependency_expression=dependency_string, dependency_map=dependency_map) @@ -1037,14 +978,13 @@ def resolve_pending_dependencies(self) -> None: # Clean up temporary attributes delattr(self, '_pending_dependency_string') - if hasattr(self, '_pending_dependency_map_dependency_ids'): - delattr(self, '_pending_dependency_map_dependency_ids') - if hasattr(self, '_pending_dependency_map_unique_names'): - delattr(self, '_pending_dependency_map_unique_names') + delattr(self, '_pending_dependency_map_dependency_ids') - def _find_parameter_by_dependency_id(self, dependency_id: str) -> Optional['Parameter']: + def _find_parameter_by_dependency_id(self, dependency_id: str) -> Optional['DescriptorNumber']: """Find a parameter by its dependency_id from all parameters in the global map.""" for obj in self._global_object.map._store.values(): - if isinstance(obj, Parameter) and hasattr(obj, '__dependency_id') and obj.__dependency_id == dependency_id: - return obj + if isinstance(obj, DescriptorNumber) and hasattr(obj, '_DescriptorNumber__dependency_id'): + if obj._DescriptorNumber__dependency_id == dependency_id: + return obj return None + \ No newline at end of file diff --git a/src/easyscience/variable/parameter_dependency_resolver.py b/src/easyscience/variable/parameter_dependency_resolver.py index bbd84214..ad084e70 100644 --- a/src/easyscience/variable/parameter_dependency_resolver.py +++ b/src/easyscience/variable/parameter_dependency_resolver.py @@ -67,7 +67,7 @@ def _collect_parameters(item: Any, parameters: List[Parameter]) -> None: resolved_count += 1 except Exception as e: error_count += 1 - dependency_id = getattr(param, '__dependency_id', 'unknown') + dependency_id = getattr(param, '_DescriptorNumber__dependency_id', 'unknown') errors.append(f"Failed to resolve dependencies for parameter '{param.name}'" \ f" (unique_name: '{param.unique_name}', dependency_id: '{dependency_id}'): {e}") diff --git a/tests/unit_tests/variable/test_parameter_dependency_serialization.py b/tests/unit_tests/variable/test_parameter_dependency_serialization.py index fcfe31fa..4b741bdc 100644 --- a/tests/unit_tests/variable/test_parameter_dependency_serialization.py +++ b/tests/unit_tests/variable/test_parameter_dependency_serialization.py @@ -40,8 +40,8 @@ def test_independent_parameter_serialization(self, clear_global_map): # Should not contain dependency fields assert '_dependency_string' not in serialized - assert '_dependency_map_unique_names' not in serialized - assert serialized['_independent'] is True + assert '_dependency_map_dependency_ids' not in serialized + assert '_independent' not in serialized # Deserialize global_object.map._clear() @@ -71,7 +71,7 @@ def test_dependent_parameter_serialization(self, clear_global_map): # Should contain dependency information assert serialized['_dependency_string'] == "2 * a" - assert serialized['_dependency_map_unique_names'] == {"a": a.unique_name} + assert serialized['_dependency_map_dependency_ids'] == {"a": a._DescriptorNumber__dependency_id} assert serialized['_independent'] is False # Deserialize @@ -81,7 +81,7 @@ def test_dependent_parameter_serialization(self, clear_global_map): # Should have pending dependency info assert hasattr(new_b, '_pending_dependency_string') assert new_b._pending_dependency_string == "2 * a" - assert new_b._pending_dependency_map_unique_names == {"a": a.unique_name} + assert new_b._pending_dependency_map_dependency_ids == {"a": a._DescriptorNumber__dependency_id} assert new_b.independent is True # Initially independent until dependencies resolved def test_dependency_resolution_after_deserialization(self, clear_global_map): @@ -147,20 +147,26 @@ def test_unique_name_dependency_serialization(self, clear_global_map): a_serialized = a.as_dict() b_serialized = b.as_dict() + del a_serialized['unique_name'] # Remove unique_name to force new assignment on deserialization + del b_serialized['unique_name'] + # Should contain unique name mapping - assert b_serialized['_dependency_string'] == '2 * "Parameter_0"' - assert "__Parameter_0__" in b_serialized['_dependency_map_unique_names'] - assert b_serialized['_dependency_map_unique_names']["__Parameter_0__"] == a.unique_name + assert b_serialized['_dependency_string'] == '2 * __Parameter_0__' + assert "__Parameter_0__" in b_serialized['_dependency_map_dependency_ids'] + assert b_serialized['_dependency_map_dependency_ids']["__Parameter_0__"] == a._DescriptorNumber__dependency_id # Deserialize both and resolve global_object.map._clear() - new_a = Parameter.from_dict(a_serialized) + c = Parameter(name='c', value=0.0) # Dummy to occupy unique name, to force new unique_names + new_b = Parameter.from_dict(b_serialized) + new_a = Parameter.from_dict(a_serialized) resolve_all_parameter_dependencies({"a": new_a, "b": new_b}) - + # Should work correctly assert new_b.independent is False - assert new_b.value == 6.0 # 2 * 3 + new_a.value = 4.0 + assert new_b.value == 8.0 # 2 * 4 def test_json_serialization_roundtrip(self, clear_global_map): """Test that parameter dependencies survive JSON serialization.""" @@ -247,6 +253,50 @@ def test_multiple_dependent_parameters(self, clear_global_map): assert new_params["y"].value == 10.0 # 2 * 5 assert new_params["z"].value == 15.0 # 10 + 5 + def test_dependency_with_descriptor_number(self, clear_global_map): + """Test that dependencies involving DescriptorNumber serialize correctly.""" + from easyscience.variable import DescriptorNumber + # When + + x = DescriptorNumber(name="x", value=3.0, unit="m") + y = Parameter(name="y", value=4.0, unit="m") + z = Parameter.from_dependency( + name="z", + dependency_expression="x + y", + dependency_map={"x": x, "y": y}, + ) + + # Verify original functionality + assert z.value == 7.0 # 3 + 4 + + # Then + # Serialize all + params_data = { + "x": x.as_dict(), + "y": y.as_dict(), + "z": z.as_dict() + } + # Deserialize and resolve + global_object.map._clear() + new_params = {} + for name, data in params_data.items(): + if name == "x": + new_params[name] = DescriptorNumber.from_dict(data) + else: + new_params[name] = Parameter.from_dict(data) + + resolve_all_parameter_dependencies(new_params) + + # Expect + # Test that functionality still works + assert new_params["z"].value == 7.0 # 3 + 4 + new_x = new_params["x"] + new_y = new_params["y"] + new_x.value = 4.0 + assert new_params["z"].value == 8.0 # 4 + 4 + new_y.value = 6.0 + assert new_params["z"].value == 10.0 # 4 + 6 + def test_get_parameters_with_pending_dependencies(self, clear_global_map): """Test utility function for finding parameters with pending dependencies.""" # Create parameters @@ -295,7 +345,7 @@ def test_error_handling_missing_dependency(self, clear_global_map): new_b = Parameter.from_dict(b_data) # Should raise error when trying to resolve - with pytest.raises(ValueError, match="Error resolving dependency"): + with pytest.raises(ValueError, match="Cannot find parameter with dependency_id"): new_b.resolve_pending_dependencies() def test_backward_compatibility_base_deserializer(self, clear_global_map): @@ -323,8 +373,15 @@ def test_backward_compatibility_base_deserializer(self, clear_global_map): assert deserialized.name == "b" assert deserialized.independent is True # Base path doesn't handle dependencies - def test_dependency_id_system_order_independence(self, clear_global_map): + @pytest.mark.parametrize("order", [ + ["x", "y", "z"], + ["z", "x", "y"], + ["y", "z", "x"], + ["z", "y", "x"] + ], ids=['normal_order', 'dependent_first', 'mixed_order', 'dependent_first_reverse']) + def test_dependency_id_system_order_independence(self, clear_global_map, order): """Test that dependency IDs allow parameters to be loaded in any order.""" + # WHEN # Create parameters with dependencies x = Parameter(name="x", value=5.0, unit="m", min=0, max=20) y = Parameter(name="y", value=10.0, unit="m", min=0, max=30) @@ -340,9 +397,8 @@ def test_dependency_id_system_order_independence(self, clear_global_map): assert z.value == 50.0 # 5 * 10 # Get dependency IDs - x_dep_id = x.dependency_id - y_dep_id = y.dependency_id - z_dep_id = z.dependency_id + x_dep_id = x._DescriptorNumber__dependency_id + y_dep_id = y._DescriptorNumber__dependency_id # Serialize all parameters params_data = { @@ -354,76 +410,35 @@ def test_dependency_id_system_order_independence(self, clear_global_map): # Verify dependency IDs are in serialized data assert params_data["x"]["__dependency_id"] == x_dep_id assert params_data["y"]["__dependency_id"] == y_dep_id - assert params_data["z"]["__dependency_id"] == z_dep_id + assert "__dependency_id" not in params_data["z"] assert "_dependency_map_dependency_ids" in params_data["z"] - # Test loading in different orders - test_orders = [ - ["x", "y", "z"], # Normal order - ["z", "x", "y"], # Dependent first - ["y", "z", "x"], # Mixed order - ["z", "y", "x"] # Dependent first, reverse order - ] - - for order in test_orders: - global_object.map._clear() - new_params = {} - - # Load in the specified order - for name in order: - new_params[name] = Parameter.from_dict(params_data[name]) - - # Verify dependency IDs are preserved - assert new_params["x"].dependency_id == x_dep_id - assert new_params["y"].dependency_id == y_dep_id - assert new_params["z"].dependency_id == z_dep_id - - # Resolve dependencies - resolve_all_parameter_dependencies(new_params) - - # Verify functionality regardless of loading order - assert new_params["z"].independent is False - assert new_params["z"].value == 50.0 - - # Test dependency updates still work - new_params["x"].value = 6.0 - assert new_params["z"].value == 60.0 # 6 * 10 - - new_params["y"].value = 8.0 - assert new_params["z"].value == 48.0 # 6 * 8 + # THEN + global_object.map._clear() + new_params = {} - def test_dependency_id_fallback_to_unique_name(self, clear_global_map): - """Test that the system falls back to unique_name when dependency_id is not available.""" - # Create parameters - a = Parameter(name="a", value=3.0, unit="m") - b = Parameter.from_dependency( - name="b", - dependency_expression="2 * a", - dependency_map={"a": a}, - unit="m" - ) + # Load in the specified order + for name in order: + new_params[name] = Parameter.from_dict(params_data[name]) - # Serialize parameters - params_data = { - "a": a.as_dict(), - "b": b.as_dict() - } + # EXPECT + # Verify dependency IDs are preserved + assert new_params["x"]._DescriptorNumber__dependency_id == x_dep_id + assert new_params["y"]._DescriptorNumber__dependency_id == y_dep_id - # Remove dependency_id information to simulate old format - del params_data["b"]["_dependency_map_dependency_ids"] - # Keep unique_name mapping for fallback + # Resolve dependencies + resolve_all_parameter_dependencies(new_params) - # Deserialize and resolve - global_object.map._clear() - new_params = {} - for name, data in params_data.items(): - new_params[name] = Parameter.from_dict(data) + # Verify functionality regardless of loading order + assert new_params["z"].independent is False + assert new_params["z"].value == 50.0 - resolve_all_parameter_dependencies(new_params) + # Test dependency updates still work + new_params["x"].value = 6.0 + assert new_params["z"].value == 60.0 # 6 * 10 - # Should still work using unique_name fallback - assert new_params["b"].independent is False - assert new_params["b"].value == 6.0 # 2 * 3 + new_params["y"].value = 8.0 + assert new_params["z"].value == 48.0 # 6 * 8 def test_deserialize_and_resolve_parameters_helper(self, clear_global_map): """Test the convenience helper function for deserialization and dependency resolution.""" @@ -482,28 +497,3 @@ def test_deserialize_and_resolve_parameters_helper(self, clear_global_map): pending = get_parameters_with_pending_dependencies(new_params) assert len(pending) == 0 - def test_serialization_error_dependency_without_unique_name(self, clear_global_map): - """Test that serialization raises an appropriate error when a dependency object lacks unique_name.""" - # Create a parameter with dependencies - a = Parameter(name="a", value=2.0, unit="m", min=0, max=10) - b = Parameter.from_dependency( - name="b", - dependency_expression="2 * a", - dependency_map={"a": a}, - unit="m" - ) - - # Artificially create a mock object without unique_name in the dependency map - # This is a defensive test since under normal circumstances this shouldn't happen - mock_obj = Mock() - # Mock obj has no unique_name attribute - if hasattr(mock_obj, 'unique_name'): - delattr(mock_obj, 'unique_name') - - # Manually inject the mock object into the dependency map to trigger the error - # This bypasses the normal type checking that would prevent this - b._dependency_map["mock"] = mock_obj - - # Attempt to serialize should raise a ValueError - with pytest.raises(ValueError, match="does not have a unique_name attribute"): - b.as_dict()