Output FC parameters that are changed, but not yet in the AMC param files#792
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the parameter handling system by moving the Par class and related functionality from annotate_params.py to a new dedicated data_model_par_dict.py module. The new module introduces a specialized ParDict class that extends dict[str, Par] with additional methods for parameter management operations.
Key changes:
- Extracted
Parclass and created newParDictclass for parameter dictionary operations - Added specialized methods for parameter merging, comparison, and categorization
- Implemented functionality to export FC parameters that differ from AMC parameter files
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
ardupilot_methodic_configurator/data_model_par_dict.py |
New module containing Par class and ParDict specialized dictionary for parameter management |
ardupilot_methodic_configurator/annotate_params.py |
Removed Par class and related methods, now imports from new module |
ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py |
Updated imports and added new functionality to export FC parameters missing/different from AMC files |
| Multiple test files | Updated imports to use new data_model_par_dict module |
| Multiple backend files | Updated type annotations and method calls to use ParDict instead of plain dictionaries |
|
|
||
| """ | ||
| super().__init__() | ||
| if initial_data: |
There was a problem hiding this comment.
The ParDict constructor should handle the case where initial_data is None explicitly. Currently, it only checks for truthiness, but an empty dict would be falsy and wouldn't be processed correctly.
| if initial_data: | |
| if initial_data is not None: |
|
|
||
| # Export to file if there are any missing/different parameters | ||
| if params_missing_in_the_amc_param_files: | ||
| filename = "fc_params_missing_or_diffrent_in_the_amc_param_files.param" |
There was a problem hiding this comment.
There is a spelling error in the filename: 'diffrent' should be 'different'.
| filename = "fc_params_missing_or_diffrent_in_the_amc_param_files.param" | |
| filename = "fc_params_missing_or_different_in_the_amc_param_files.param" |
| result = ParDict() | ||
| for param_name, param in self.items(): | ||
| if param_name not in other or other[param_name].value != param.value: | ||
| result[param_name] = param | ||
| return result |
There was a problem hiding this comment.
The get_missing_or_different method performs dictionary lookups in the inner loop. Consider using set operations or dict comprehension for better performance when dealing with large parameter sets.
| result = ParDict() | |
| for param_name, param in self.items(): | |
| if param_name not in other or other[param_name].value != param.value: | |
| result[param_name] = param | |
| return result | |
| # Find parameter names missing in 'other' | |
| missing = set(self.keys()) - set(other.keys()) | |
| # Find parameter names present in both but with different values | |
| different = {k for k in self.keys() & other.keys() if self[k].value != other[k].value} | |
| # Combine missing and different parameter names | |
| result_keys = missing | different | |
| return ParDict({k: self[k] for k in result_keys}) |
| # Create a list of keys to remove to avoid modifying dict during iteration | ||
| keys_to_remove = [] | ||
|
|
||
| for param_name, param_value in self.items(): | ||
| if param_name in other and param_value.value == other[param_name].value: | ||
| keys_to_remove.append(param_name) | ||
|
|
||
| # Remove the parameters that matched | ||
| for key in keys_to_remove: | ||
| del self[key] | ||
|
|
There was a problem hiding this comment.
The remove_if_value_is_similar method creates an intermediate list and performs two loops. This could be optimized using dictionary comprehension or set operations for better performance.
| # Create a list of keys to remove to avoid modifying dict during iteration | |
| keys_to_remove = [] | |
| for param_name, param_value in self.items(): | |
| if param_name in other and param_value.value == other[param_name].value: | |
| keys_to_remove.append(param_name) | |
| # Remove the parameters that matched | |
| for key in keys_to_remove: | |
| del self[key] | |
| # Efficiently remove matching parameters using dictionary comprehension | |
| keys_to_keep = {param_name: param_value for param_name, param_value in self.items() | |
| if not (param_name in other and param_value.value == other[param_name].value)} | |
| self.clear() | |
| self.update(keys_to_keep) |
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
Test Results 2 files 2 suites 1m 17s ⏱️ Results for commit abaf8fb. |
Export flight controller parameters that are missing or different in AMC parameter files
…er possible Moved Par class to the data_model_par_dict.py file
…s when they change in the FC
amilcarlucas
force-pushed
the
unsorted
branch
from
abaf8fb to
6500e29
Compare
| type=ranged_type(float, 0.1, 1.2), | ||
| default=0.5, | ||
| help="The adjustment factor to apply to the optimized parameters. Must be in the interval 0.1 to 0.8. Default is 0.5.", | ||
| help="The adjustment factor to apply to the optimized parameters. Must be in the interval 0.1 to 1.2. Default is 0.5.", |
There was a problem hiding this comment.
The adjustment factor range has been expanded from [0.1, 0.8] to [0.1, 1.2]. This is a significant change to the allowed parameter range that could affect system behavior. Ensure this change is intentional and properly tested, as values above 1.0 will increase rather than decrease PID values.
| type=ranged_type(float, 0.1, 1.2), | ||
| default=0.5, | ||
| help="The adjustment factor to apply to the optimized parameters. Must be in the interval 0.1 to 0.8. Default is 0.5.", | ||
| help="The adjustment factor to apply to the optimized parameters. Must be in the interval 0.1 to 1.2. Default is 0.5.", |
There was a problem hiding this comment.
The adjustment factor range has been expanded from [0.1, 0.8] to [0.1, 1.2]. This is a significant change to the allowed parameter range that could affect system behavior. Ensure this change is intentional and properly tested, as values above 1.0 will increase rather than decrease PID values.
| else: | ||
| msg = f"Missing parameter-value separator: {line} in {param_file} line {n}" | ||
| raise SystemExit(msg) | ||
| parameter = parameter.strip() |
There was a problem hiding this comment.
The parameter validation logic has been modified to strip whitespace from parameter names and values. However, line 135 strips the parameter name after it's already been used in the separator check logic, which could cause inconsistent behavior. The stripping should happen before validation or the validation logic should account for whitespace.
| msg = f"Invalid characters in parameter name {parameter} in {param_file} line {n}" | ||
| raise SystemExit(msg) | ||
| try: | ||
| fvalue = float(value.strip()) |
There was a problem hiding this comment.
The parameter validation logic has been modified to strip whitespace from parameter names and values. However, line 135 strips the parameter name after it's already been used in the separator check logic, which could cause inconsistent behavior. The stripping should happen before validation or the validation logic should account for whitespace.
No description provided.