Fix clustering and model update #35
Conversation
Clustering and mode tracking is added with example code for both and a shared module for both. Other small changes have been made.
Clustering and mode tracking is added with example code for both and a shared module for both. Other small changes have been made.
Splitting function blocks into smaller functions. Other small changes.
Fixed an error in create_cluster, that resulted in doublicate model orders for a cluster. Allowed for scatter point size to be adjusted.
Removed debug info from some files, added missing type info for function arguments and returns, updated to YaFEM 1.0.0, changed some function descriptions, added timestamp to clustering data
Add model update module. Added reconnect function for MQTT client. Added timestamp outputs to some functions. Added models folder to store YAFEM models and model parameters. Added function to publish clustering results. Small change to the mode tracking algorithm. Added function to check mode shapes. Moved convert_list function to util.py, since it is used in multiple other functions. Added function to view the spectral density. Changes to readability.
Added example functions
dict to Dict, list to List, tuple to Tuple
There was a problem hiding this comment.
Pull Request Overview
This PR adds model updating functionality and makes several improvements to the codebase, including mode clustering enhancements, reconnection capabilities, and timestamp tracking. The changes support a full pipeline from system identification through clustering to model updating with MQTT-based communication.
- Added model update package with FEM-based model updating using YaFEM
- Added reconnect function for MQTT clients to handle disconnections
- Enhanced mode tracking and clustering with improved parameter handling and type hints
- Added timestamp tracking to function returns for better traceability
Reviewed Changes
Copilot reviewed 34 out of 38 changed files in this pull request and generated 31 comments.
Show a summary per file
| File | Description |
|---|---|
| src/methods/sysid.py | Added numpy import, improved type hints, added reconnect functionality, updated return signature to include timestamp |
| src/methods/packages/models/beam_yafem_model.py | New file implementing beam finite element model using YaFEM library |
| src/methods/packages/eval_yafem_model.py | Removed old model implementation (replaced by beam_yafem_model.py) |
| src/methods/model_update_functions/model_update_func.py | New file implementing model parameter estimation and optimization |
| src/methods/model_update_functions/mode_pairing.py | New file implementing mode pairing between measured and model modes |
| src/methods/model_update.py | Complete refactor with new functions for live model updating with remote clustering |
| src/methods/mode_update_functions/* | Removed old mode update implementation files |
| src/methods/mode_tracking*.py | Enhanced type hints, improved parameter naming consistency, added isinstance checks |
| src/methods/mode_clustering*.py | Enhanced type hints, added publish functionality, improved error handling |
| src/functions/util.py | Added _convert_list_to_dict_or_array helper function |
| src/functions/spectral_density.py | New file for plotting spectral density |
| src/functions/plot_model_update.py | New file for plotting model update results |
| src/data/comm/mqtt.py | Added reconnect_client function |
| src/examples/* | Updated example scripts to use new model update functions |
Comments suppressed due to low confidence (1)
src/methods/model_update.py:118
- Except block directly handles BaseException.
except:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/methods/mode_tracking_functions/match_to_tracked_cluster.py
Outdated
Show resolved
Hide resolved
src/methods/mode_tracking_functions/match_to_tracked_cluster.py
Outdated
Show resolved
Hide resolved
prasadtalasila
left a comment
prasadtalasila
left a comment
There was a problem hiding this comment.
@au650680 Thanks for this PR. Please see the comments and consider them if useful. In addition, I have the following observations
Please increase the version in pyproject.toml to 0.6.0
The MQTT topic(s) on which a block publishes need to be taken from the configuration.
The respective json key could be "TopicsToPublish".
The model name could be taken from the configuration as well.
Github is showing some spelling mistakes in the new code. Please see this page for further details.
#35
Can you please take a look at the suggestions for improving the code quality below?
https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&sinceLeakPeriod=true&pullRequest=35&id=INTO-CPS-Association_example-shm
There are many suggestions, it would be great even if some of them are addressed.
Please ignore the following warning though:
Rename this parameter "Params" to match the regular expression ^[a-z][a-z0-9]*$.
There are some duplicate blocks of code that could be made into reusable functions or methods to enhance maintainability and reduce redundancy.
Please see the details here:
https://sonarcloud.io/component_measures?id=INTO-CPS-Association_example-shm&metric=duplicated_lines_density&view=list
The following experiment is giving a model update warning when trying to run it:
$example-shm live-model-update-remote-sysid
JSON configuration loaded successfully.
Waiting for sysid data...
Connected to MQTT broker.
Subscribed to topic: cpsens/d8-3a-dd-37-d2-7e/3050-A-060_sn_106209/1/acc/sysid/data
on_subscribe: Subscription ID 1 with QoS levels [ReasonCode(Suback, 'Granted QoS 0')]
Message received on topic: cpsens/d8-3a-dd-37-d2-7e/3050-A-060_sn_106209/1/acc/sysid/data
Received sysid data at timestamp: 2025-11-12T18:26:45.087951
Sysid data received. Running mode clustering...
Cluster 46.58498001098633 too short: 2 Must be: > 6
Cluster 60.4477653503418 too short: 2 Must be: > 6
Cluster saved: 74.61399459838867
Cluster 118.81608581542969 too short: 4 Must be: > 6
Cluster 0.07243721187114716 too short: 3 Must be: > 6
Cluster 60.44617462158203 too short: 5 Must be: > 6
Cluster 0.07284276187419891 too short: 1 Must be: > 6
Clustered frequencies [74.6139946]
Model parameters loaded succesfully from: 2025-11-12T16:59:15.125911
Cluster 0 74.61399459838867 is not matched. Reason: MAC threshold
Skipping model updating due to error: The problem becomes undetermined. The number of updated parameters should not be more than the number of features
Model frequencies: None [Hz]
Updated parameters are:
JSON configuration loaded successfully.
Waiting for sysid data...
Connected to MQTT broker.
Subscribed to topic: cpsens/d8-3a-dd-37-d2-7e/3050-A-060_sn_106209/1/acc/sysid/data
on_subscribe: Subscription ID 1 with QoS levels [ReasonCode(Suback, 'Granted QoS 0')]
Message received on topic: cpsens/d8-3a-dd-37-d2-7e/3050-A-060_sn_106209/1/acc/sysid/data
Received sysid data at timestamp: 2025-11-12T18:27:45.060819
Sysid data received. Running mode clustering...
Cluster 46.637929916381836 too short: 2 Must be: > 6
Cluster 60.38414764404297 too short: 4 Must be: > 6
Cluster saved: 74.81135559082031
Cluster 117.53659057617188 too short: 1 Must be: > 6
Cluster 60.382686614990234 too short: 2 Must be: > 6
Cluster 118.45673370361328 too short: 1 Must be: > 6
Cluster 0.043501995503902435 too short: 2 Must be: > 6
Clustered frequencies [74.81135559]
Model parameters loaded succesfully from: 2025-11-12T16:59:15.125911
Cluster 0 74.81135559082031 is not matched. Reason: MAC threshold
Skipping model updating due to error: The problem becomes undetermined. The number of updated parameters should not be more than the number of features
Model frequencies: None [Hz]
Updated parameters are:
JSON configuration loaded successfully.
Waiting for sysid data...
Connected to MQTT broker.
Subscribed to topic: cpsens/d8-3a-dd-37-d2-7e/3050-A-060_sn_106209/1/acc/sysid/data
on_subscribe: Subscription ID 1 with QoS levels [ReasonCode(Suback, 'Granted QoS 0')]
Message received on topic: cpsens/d8-3a-dd-37-d2-7e/3050-A-060_sn_106209/1/acc/sysid/data
Received sysid data at timestamp: 2025-11-12T18:28:45.129166
Sysid data received. Running mode clustering...
Cluster 46.52902412414551 too short: 2 Must be: > 6
Cluster 60.41178321838379 too short: 2 Must be: > 6
Cluster saved: 74.71917343139648
Cluster 118.93480682373047 too short: 2 Must be: > 6
Cluster 0.08876887708902359 too short: 1 Must be: > 6
Cluster 60.40863609313965 too short: 4 Must be: > 6
Cluster 0.016485288739204407 too short: 1 Must be: > 6
Clustered frequencies [74.71917343]
Model parameters loaded succesfully from: 2025-11-12T16:59:15.125911
Cluster 0 74.71917343139648 is not matched. Reason: MAC threshold
Skipping model updating due to error: The problem becomes undetermined. The number of updated parameters should not be more than the number of features
Model frequencies: None [Hz]
Updated parameters are:
JSON configuration loaded successfully.
Waiting for sysid data...
Connected to MQTT broker.
Subscribed to topic: cpsens/d8-3a-dd-37-d2-7e/3050-A-060_sn_106209/1/acc/sysid/data
on_subscribe: Subscription ID 1 with QoS levels [ReasonCode(Suback, 'Granted QoS 0')]I am not sure if this is expected.
The programs that are subscribing to MQTT seem to run indefinitely. This may be an issue.
Mohamed resolved this issue in this pull request
#17
and the corresponding commit is this.
2a6b0fe
Hope it can help resolve the issue.
please keep the type hints in the code as much as possible. They help a lot with code understanding and maintenance.
Each function block (mode clustering, model updating, etc.) seem to have mqtt code inside. It might be better to have dedicated callback functions that can either be void or MQTT client. If it is MQTT client, then the function can subscribe to the relevant topics inside the function. This will help separate the MQTT code from the actual logic of the function.
The suggested design also helps separate the MongoDB, MQTT bits from the core algorithmic logic.
|
@prasadtalasila, I have just made a new commit, which resolve most of your comments.
|
- Added TopicToPublish to config file. - Reduced code by reusing dublicate code. - Added missing docstrings and argument types. - Removed unused variables - Small changes to plotting
- Duplicate code have been reduced. - Models folder have been moved to top directory. - mqtt.start_mqtt(), mqtt.shutdown() and mqtt.publish_to_mqtt() is added. - Config have been change to have both MetadataToSubscribe, TopicsToSubscribe and TopicsToPublish. - Topic indexes is not necessary to supply subscribe and publish functions. - Publish functions have will now handle errors with missing topics. - natural_freq.py have been replaced by plot_sysid.py. - README.md is updated. - Redundant sys.stdout.flush() have been removed. - Added plot titles.
Added placeholder names to TopicsToPublish
prasadtalasila
left a comment
prasadtalasila
left a comment
There was a problem hiding this comment.
@au650680 Thanks for the updates. Please see the comments.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 47 out of 50 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- 10 minutes of recorded beam data - Small changes to broker information - Updated replay config
- Added beam_pars.jsonl as example.
- Recordings are now done to one file. - Replay function now uses threading.Timer() to delay the messages correctly. - New path to model folder
Removed while loop
- Edit to docstring - Edit to type hint - Timestamp replaced with aligner time in sysid.py
|
@au650680 , The $poetry build
Building example-shm (0.6.0)
Building sdist
- Building sdist
- Built example_shm-0.6.0.tar.gz
Building wheel
- Building wheel
C:\Python312\Lib\zipfile\__init__.py:1598: UserWarning: Duplicate name: '__init__.py'
return self._open_to_write(zinfo, force_zip64=force_zip64)
- Built example_shm-0.6.0-py3-none-any.whlMaking the following changes fixes the problem.
packages = [
{include = "data", from="src"},
{include = "examples", from="src"},
{include = "functions", from="src"},
{include = "methods", from="src"},
{include = "beam", from="models"}
] |
|
@au650680 the following error exists in the code base $example-shm accelerometers
JSON configuration loaded successfully.
Traceback (most recent call last):
File "C:\Users\foogit\example-shm\.venv\Scripts\\example-shm", line 6, in <module>
sys.exit(cli())
^^^^^
File "C:\Users\foogit\example-shm\.venv\Lib\site-packages\click\core.py", line 1462, in __call__
return self.main(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\foogit\example-shm\.venv\Lib\site-packages\click\core.py", line 1383, in main
rv = self.invoke(ctx)
^^^^^^^^^^^^^^^^
File "C:\Users\foogit\example-shm\.venv\Lib\site-packages\click\core.py", line 1850, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\foogit\example-shm\.venv\Lib\site-packages\click\core.py", line 1246, in invoke
return ctx.invoke(self.callback, **ctx.params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\foogit\example-shm\.venv\Lib\site-packages\click\core.py", line 814, in invoke
return callback(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\foogit\example-shm\.venv\Lib\site-packages\click\decorators.py", line 34, in new_func
return f(get_current_context(), *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\foogit\example-shm\src\examples\example.py", line 38, in accelerometers
read_accelerometers(ctx.obj["CONFIG"])
File "C:\Users\foogit\example-shm\src\examples\acceleration_readings.py", line 12, in read_accelerometers
accelerometer = Accelerometer(
^^^^^^^^^^^^^^
File "C:\Users\foogit\example-shm\src\data\accel\hbk\accelerometer.py", line 34, in __init__
self.mqtt_client.subscribe(self.topic, qos=1)
File "C:\Users\foogit\example-shm\.venv\Lib\site-packages\paho\mqtt\client.py", line 2014, in subscribe
on_connect: Connected with response code Success
for t, o in topic:
Subscribing to topic: cpsens/d8-3a-dd-37-d2-7e/3050-A-060_sn_106209/1/acc/raw/data
^^^^
ValueError: too many values to unpack (expected 2) |
prasadtalasila
left a comment
prasadtalasila
left a comment
There was a problem hiding this comment.
@au650680 please see this minor comment
- Reverted sysid config back to MQTT to have a more a general name - Fixed warning with poetry build - Fixed error within acceleration_readings() - Added possibility to loop replay function.
- Changed key from "MQTT" back to "sysid" for configuration and code
- YAFEM model function which imported from the model file is now set in constants file, similar to other model variables.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 57 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (1)
USERGUIDE.md:1
- Typo in topic string appears to have garbled text 'model_clmodel_updateuster'.
# User Guide
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
- Added specific replay.json.template for use with record and replay files. - Added replay_production.json.template for running function blocks with replayed data. - Added more generalized model update information for the function block to grab from constants.py - Updated USERGUIDE.md - Typos, type hints and other minor improvements
The recording topic is now the same as the subscribe topic, which allows the user to see the origin of the recorded data.
2 participants
Fixed merge conflicts.