-
Notifications
You must be signed in to change notification settings - Fork 4
Refactor 24 trend new #216
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,50 +1,182 @@ | ||
| import logging | ||
| from dataclasses import dataclass | ||
|
|
||
| import numpy as np | ||
|
|
||
| from sasdata.data import SasData, SasMeasurement | ||
| from sasdata.data_backing import Dataset, Group | ||
| from sasdata.quantities.quantity import Quantity | ||
| from sasdata.transforms.rebinning import calculate_interpolation_matrix_1d | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Axis strs refer to the name of their associated NamedQuantity. | ||
|
|
||
|
|
||
| # TODO: This probably shouldn't be here but will keep it here for now. | ||
| # TODO: This probably shouldn't be here but will keep it here for now. --> In sasdta/data.py? | ||
| # TODO: Similarity/relation to __getitem__ in SasData class? | ||
| # TODO: Or a method of Metadata class? | ||
| # TODO: Not sure how to type hint the return. | ||
| def get_metadatum_from_path(data: SasData, metadata_path: list[str]): | ||
| current_group = data._raw_metadata | ||
| current_node = data.metadata.raw | ||
| for path_item in metadata_path: | ||
| current_item = current_group.children.get(path_item, None) | ||
| if current_item is None or (isinstance(current_item, Dataset) and path_item != metadata_path[-1]): | ||
| raise ValueError("Path does not lead to valid a metadatum.") | ||
| elif isinstance(current_item, Group): | ||
| current_group = current_item | ||
| else: | ||
| return current_item.data | ||
| current_item = None | ||
|
|
||
| if isinstance(current_node.contents, list): | ||
| # Search through list of MetaNodes | ||
| for node in current_node.contents: | ||
| if node.name == path_item: | ||
| current_item = node | ||
| break | ||
|
|
||
| # If we did not find the item (either not a list or not found in list) | ||
| if current_item is None: | ||
| raise ValueError("Path does not lead to a valid metadatum.") | ||
|
|
||
| # Check if we're at the end of the path | ||
| if path_item == metadata_path[-1]: | ||
| return current_item.contents | ||
|
|
||
| current_node = current_item | ||
| raise ValueError("End of path without finding a dataset.") | ||
|
|
||
|
|
||
| @dataclass | ||
| class Trend: | ||
| data: list[SasData] | ||
| # This is going to be a path to a specific metadatum. | ||
| # | ||
| # TODO: But what if the trend axis will be a particular NamedQuantity? Will probably need to think on this. | ||
| trend_axis: list[str] | ||
|
|
||
| # Designed to take in a particular value of the trend axis, and return the SasData object that matches it. | ||
| # TODO: Not exaclty sure what item's type will be. It could depend on where it is pointing to. | ||
| def __getitem__(self, item) -> SasData: | ||
| for datum in self.data: | ||
| metadatum = get_metadatum_from_path(datum, self.trend_axis) | ||
| if metadatum == item: | ||
| return datum | ||
| raise KeyError() | ||
| trend_axes: dict[str, list[str] | list] # Path or manual values | ||
|
|
||
| def __post_init__(self): | ||
|
|
||
| # First, filter out invalid data items | ||
| self._filter_and_validate_data() | ||
|
|
||
| # Validate data length matches manual value lists | ||
| self._validate_manual_values() | ||
|
|
||
| # Validate metadata paths | ||
| self._validate_metadata_paths() | ||
|
|
||
| def _filter_and_validate_data(self): | ||
| """Filter out non-SasData objects and validate data integrity""" | ||
| valid_data = [] | ||
| invalid_indices = [] | ||
|
|
||
| for i, datum in enumerate(self.data): | ||
| if not isinstance(datum, SasData): | ||
| invalid_indices.append(i) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ty (a Python type checker) is complaining that this code is unreachable, probably because |
||
| continue | ||
|
|
||
| # Check if datum has metadata | ||
| if not hasattr(datum, "metadata") or datum.metadata is None: | ||
| invalid_indices.append(i) | ||
| continue | ||
|
|
||
| # Check if datum has raw metadata | ||
| if not hasattr(datum.metadata, "raw") or datum.metadata.raw is None: | ||
| invalid_indices.append(i) | ||
| continue | ||
|
|
||
| valid_data.append(datum) | ||
|
|
||
| # Update data with only valid items | ||
| self.data = valid_data | ||
|
|
||
| # Warn about filtered items | ||
| if invalid_indices: | ||
| logger.warning( | ||
| f"Warning: Removed data items at indices {invalid_indices} - not SasData objects or missing/invalid metadata" | ||
| ) | ||
|
|
||
| # Additional validation | ||
| if not self.data: | ||
| raise ValueError("No valid data items remain after filtering") | ||
|
|
||
| if len(self.data) < 2: | ||
| logger.warning(f"Only {len(self.data)} valid data items remain") | ||
|
|
||
| # TODO: Decide if these limitations are ok or not (e.g. Should the user be able | ||
| # to specify manual values that are not numbers? Or have a different number of | ||
| # manual values than data items? How to assign the values then?, etc.) | ||
| def _validate_manual_values(self): | ||
| """Ensure manual value lists are valid and match data length""" | ||
|
|
||
| for axis_name, axis_config in self.trend_axes.items(): | ||
| # Only validate if this is a manual value axis (not a metadata path) | ||
| if isinstance(axis_config, list) and len(axis_config) > 0 and isinstance(axis_config[0], str): | ||
| # This is a metadata path, skip manual value validation | ||
| continue | ||
|
|
||
| if not isinstance(axis_config, list): | ||
| raise ValueError( | ||
| f"Manual values for axis '{axis_name}' should be passed as a list, got {type(axis_config).__name__}" | ||
| ) | ||
|
|
||
| if len(axis_config) == 0: | ||
| raise ValueError(f"Manual values for axis '{axis_name}' must not be empty") | ||
|
|
||
| if not all(isinstance(v, (int, float)) for v in axis_config): | ||
| raise ValueError(f"All values for axis '{axis_name}' must be numbers (int or float)") | ||
|
|
||
| if len(axis_config) != len(self.data): | ||
| raise ValueError( | ||
| f"Manual values for axis '{axis_name}' must have same length as data " | ||
| f"({len(self.data)} items, got {len(axis_config)})" | ||
| ) | ||
|
|
||
| def _validate_metadata_paths(self): | ||
| """Validate metadata paths""" | ||
| for axis_name, axis_config in self.trend_axes.items(): | ||
| if isinstance(axis_config, list) and len(axis_config) > 0 and isinstance(axis_config[0], str): | ||
| # This is a metadata path | ||
| for i, datum in enumerate(self.data): | ||
| try: | ||
| get_metadatum_from_path(datum, axis_config) | ||
| except ValueError as e: | ||
| raise ValueError(f"trend_axes['{axis_name}'] path {axis_config} invalid for data item {i}: {e}") | ||
|
|
||
| def get_trend_values(self, axis_name: str) -> list: | ||
| """Get values for a named trend axis""" | ||
| if axis_name not in self.trend_axes: | ||
| raise KeyError(f"Axis '{axis_name}' not found") | ||
|
|
||
| axis_config = self.trend_axes[axis_name] | ||
|
|
||
| if isinstance(axis_config[0], str): | ||
| # Metadata path - extract from data | ||
| return [get_metadatum_from_path(datum, axis_config) for datum in self.data] | ||
| else: | ||
| # Manual values - return as-is | ||
| return axis_config.copy() # Return copy to prevent modification | ||
|
|
||
| def add_manual_axis(self, axis_name: str, values: list): | ||
| """Add a new manual trend axis""" | ||
| if len(values) != len(self.data): | ||
| raise ValueError(f"Manual values must have same length as data ({len(self.data)} items, got {len(values)})") | ||
|
|
||
| self.trend_axes[axis_name] = values.copy() | ||
|
|
||
| def add_metadata_axis(self, axis_name: str, path: list[str]): | ||
| """Add a new metadata trend axis""" | ||
| # Validate the path first | ||
| for i, datum in enumerate(self.data): | ||
| try: | ||
| get_metadatum_from_path(datum, path) | ||
| except ValueError as e: | ||
| raise ValueError(f"Path {path} invalid for data item {i}: {e}") | ||
|
|
||
| self.trend_axes[axis_name] = path | ||
|
|
||
| @property | ||
| def trend_axes(self) -> list[float]: | ||
| return [get_metadatum_from_path(datum, self.trend_axis) for datum in self.data] | ||
| def axis_names(self) -> list[str]: | ||
| return list(self.trend_axes.keys()) | ||
|
|
||
| def is_manual_axis(self, axis_name: str) -> bool: | ||
| """Check if an axis uses manual values or metadata path""" | ||
| if axis_name not in self.trend_axes: | ||
| raise KeyError(f"Axis '{axis_name}' not found") | ||
|
|
||
| axis_config = self.trend_axes[axis_name] | ||
| return not (isinstance(axis_config, list) and len(axis_config) > 0 and isinstance(axis_config[0], str)) | ||
|
|
||
| # TODO: Assumes there are at least 2 items in data. Is this reasonable to assume? Should there be error handling for | ||
| # situations where this may not be the case? | ||
|
|
@@ -86,5 +218,5 @@ def interpolate(self, axis: str) -> "Trend": | |
| metadata=datum.metadata, | ||
| ) | ||
| new_data.append(new_datum) | ||
| new_trend = Trend(new_data, self.trend_axis) | ||
| new_trend = Trend(new_data, self.trend_axes) | ||
| return new_trend | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This match isn't completely equivalent to the previous if/else block. If
self.contents == "", the new check will givechildren == f"\n{header} {self.contents}", but gave an empty string before.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just restored the old behaviour by adding a check for an empty string in the case. I'm not exactly sure which is the best, but that might be a decision to make later down the line.