Implemented unit conversion support for KeyValueStoreSolver#30
Open
tombonfert wants to merge 10 commits into
Open
Implemented unit conversion support for KeyValueStoreSolver#30tombonfert wants to merge 10 commits into
tombonfert wants to merge 10 commits into
Conversation
- Added `unit_conversion_table` parameter to `MeasurementDBConfig` and updated related methods to handle unit conversions. - Modified `load_blob` methods across various cache classes to accept `uses_alias` parameter for compatibility with unit conversion logic. - Updated `SolverConfig` to include unit conversion mappings and properties. - Introduced unit conversion tests to validate functionality and ensure correct behavior when using aliased selectors. - Enhanced documentation and comments for clarity on unit conversion processes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #30 +/- ##
==========================================
+ Coverage 86.03% 86.28% +0.24%
==========================================
Files 52 52
Lines 4061 4177 +116
Branches 483 498 +15
==========================================
+ Hits 3494 3604 +110
- Misses 473 477 +4
- Partials 94 96 +2
🚀 New features to boost your workflow:
|
- Added `unit_conversion_table` parameter to relevant configuration sections, detailing its role in converting time-series values during `solve()`. - Enhanced documentation across multiple files to clarify the usage of `source_unit` and `target_unit` columns in the `channel_mapping` table. - Updated `KeyValueStoreSolver` documentation to reflect changes in unit conversion handling and its integration with aliased selectors. - Improved clarity on the configuration and functionality of unit conversion within the impulse framework.
…tation and code - Introduced `JoinKey` class to define custom join keys for alias resolution between `channel_mapping` and `channel_metrics`. - Updated `ChannelMappingConfig` to include an optional `join_keys` attribute, allowing for flexible alias-resolution configurations. - Enhanced `SolverConfig` to support the new `ChannelMappingConfig` and its properties. - Improved documentation to clarify the usage of `join_keys` and internal column names related to channel mapping. - Added unit tests to validate the behavior of configurable join keys and their integration with the `KeyValueStoreSolver`.
- Removed the field validator for channel mapping coercion, simplifying the handling of `ChannelMappingConfig`. - Updated tests to ensure compatibility with the new configuration, replacing instances of `TableConfig` with `ChannelMappingConfig` where applicable. - Enhanced unit tests to validate the behavior of the updated channel mapping configuration and its integration with the `KeyValueStoreSolver`.
- Modified the `unit_conversion.csv` file to add an `is_base` column, indicating whether each unit is a base unit for its group. - Updated entries for speed and rotation units to reflect their base status, enhancing clarity for unit conversion processes.
adefabian
reviewed
May 24, 2026
|
|
||
| agg_exprs = [F.flatten(F.collect_list("selector_ids")).alias("selector_ids")] | ||
| if has_unit_cols: | ||
| agg_exprs.append(F.first(source_unit_col, ignorenulls=True).alias(source_unit_col)) |
Collaborator
There was a problem hiding this comment.
What happens if we have multiple aliases on the same physical channel with different target_units?
Collaborator
Author
There was a problem hiding this comment.
yep this would be a problem. :)
I've implemented a fix now, which uses collect_set to collect available source and target units for a physical channel. In case there are > 1 source/target units available, we are raising a ValueError.
This means, right now we do not support different aliases resolving to the same physical channel, which have different source/target units defined. If this becomes a requirement in the future, the solution is to map the conversion factor to selector_id rather than to (container_id, channel_id), i..e. physical channel
adefabian
reviewed
May 24, 2026
|
|
||
| channels_df = channels_df.withColumn( | ||
| factor_col, | ||
| F.col("_src_factor") / F.col("_tgt_factor"), |
Collaborator
There was a problem hiding this comment.
currently we dont check if the value != 0 should we add something so we raise an error early on?
Collaborator
Author
There was a problem hiding this comment.
I added validation checks for the unit conversion table in _validate_unit_conversion_table
…tion - Updated the `KeyValueStoreSolver` to propagate effective `source_unit` and `target_unit` during query resolution, ensuring correct unit conversions based on `channel_metrics` and `channel_mapping`. - Enhanced documentation to clarify the behavior of `source_unit` and `target_unit`, including their precedence and fallback mechanisms. - Added new unit tests to validate the effective unit resolution logic, ensuring accurate conversions and fallback scenarios. - Refactored related code for improved clarity and maintainability.
- Updated the `KeyValueStoreSolver` to validate that each `(container_id, channel_id)` pair has at most one distinct `source_unit` and one distinct `target_unit`, preventing silent mis-conversions. - Added error handling to raise a `ValueError` when conflicting unit conversions are detected for aliased selectors, providing detailed information about the conflicts. - Enhanced documentation to clarify the constraints on unit conversions and the behavior of the solver when encountering conflicting aliases. - Introduced new unit tests to validate the conflict detection logic and ensure correct error handling for unit conversion scenarios.
- Introduced a new method `_validate_unit_conversion_table` to ensure that the `conversion_factor` in the unit conversion table is a positive non-null number, raising a `ValueError` for invalid entries. - Updated the `_compute_conversion_factors` method to call the validation method, ensuring that any malformed data is caught early in the processing pipeline. - Added comprehensive unit tests to validate the new error handling for zero, negative, and null conversion factors, providing clear feedback on invalid rows.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
unit_conversion_tableparameter toMeasurementDBConfigand updated related methods to handle unit conversions.load_blobmethods across various cache classes to acceptuses_aliasparameter for compatibility with unit conversion logic.SolverConfigto include unit conversion mappings and properties.Summary
Changes
Test Plan
Checklist