-
Notifications
You must be signed in to change notification settings - Fork 1
sysid implementation #17
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
Conversation
|
Hi @prasadtalasila I have pushed the sysid. I made experiment 3 with 2 options. The implementation assumes that topic index 1 under "MQTT" config is the metadata, it uses it to extract the Fs. |
|
@8ohamed the |
|
@prasadtalasila I have updated exp-3-plot so it terminates after getting OMA results once, and it shows the plot. |
|
@8ohamed The experiment_3_plot is not terminating even after 30 minutes. Even the experiment_3_print does not terminate. Please have a look. Thanks. |
|
Hi @prasadtalasila The experiment_3_plot and experiment_3_print are both terminating for me,. |
|
@8ohamed please paste (in your comment) the template MQTT json file. Do remember to remove the MQTT credentials. Thanks. |
|
TopicIndex 3 and 4 are for debug } |
|
@8ohamed It is working for me now with the configuration file you have given. Please update In addition, please do the following.
Thanks. |
|
Hi @prasadtalasila I have resolved the issues and added more tests. |
prasadtalasila
left a comment
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.
@8ohamed I've pushed small changes. Please see the comments.
src/data/accel/hbk/aligner.py
Outdated
| break | ||
| for ch_idx, channel_data in enumerate(entries): | ||
| if channel_data is not None: | ||
| if channel_data is not None and i < len(channel_data): |
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.
What does this line do? The logic in this function looks convoluted. Is it possible to simplify it?
src/methods/sys_id.py
Outdated
| return obj | ||
|
|
||
|
|
||
| def _on_metadata(client: MQTTClient, userdata: Dict[str, str], message) -> None: |
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 function relies heavily on metadata. Perhaps this can be moved to src/data/accel/metadata.py?
In general, any knowledge of data and metadata should be in one or two python modules so that any updates to either data or metadata formats can be easily integrated into the code base.
If we take the context of your mock_pt and hbk accelerometers, they use the same data format, right? If so, should the src/data/accel/hbk/Accelerometer.process_message() function be pushed to the IAccelerometer?
src/methods/sys_id.py
Outdated
| return FS | ||
|
|
||
|
|
||
| def setup_client(mqtt_config: Dict[str, Any]) -> MQTTClient: |
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.
You are directly importing from paho-mqtt. We already have a src/data/comm/mqtt.py. Can it be used here?
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.
The config format seems to be changing with each new feature. Perhaps having a src/util/config.py will help. This module implements the logic of config parsing while other parts of the code calls methods of this class to get the required config.
|
HI @prasadtalasila I have moved the functions out from sys_id, and resolved the issues with the tests and experiment-2. |
No description provided.