Aligner #12
Merged
Aligner #12
Conversation
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
Collaborator
Author
|
Hi @prasadtalasila I have updated the way we delete samples. If you could approve and merge this PR, I will then create PR for the sysID. |
Contributor
prasadtalasila
left a comment
prasadtalasila
left a comment
There was a problem hiding this comment.
@8ohamed This review is incomplete. I am providing the partial review in order to help you proceed further.
Contributor
Collaborator
Author
|
The reference code subscribes to metadata topic too, it extracts the datatype and the number of samples in each message. It also publishes a metadata for the aligned data, this metadata includes "UTCAtFirstSample" and "Fs" |
Contributor
prasadtalasila
left a comment
prasadtalasila
left a comment
There was a problem hiding this comment.
@8ohamed thanks for the updates to the PR. Please see the new comments.
Contributor
There was a problem hiding this comment.
Please do not update these settings. It is best to refactor the code instead.
src/data/accel/hbk/aligner.py
Outdated
| msg, acc=acc: acc.process_message(msg)) | ||
|
|
||
|
|
||
| def find_continuous_key_groups(self): |
Contributor
There was a problem hiding this comment.
please add type hints here and in other places
Contributor
There was a problem hiding this comment.
The data_map is an internal property of Accelerometer and it should not be used outside. Please write continuous_key_groups(self) for the Accelerometer and reuse those here.
Collaborator
Author
There was a problem hiding this comment.
@prasadtalasila
In order to avoid other classes accessing the data_map, I added new methods to Accelerometer
- get_sorted_keys, so each accelerometer returns a list of its keys so the aligner can use it
- get_samples_for_key so the aligner can use it to extract data, instead of directly using the data_map
- clear_used_data it deletes used data and everything that is older, aligner class calls it after extracting the data
I have pushed the changes, I will now look at the tests.
Contributor
There was a problem hiding this comment.
This test uses the actual mqtt server. Please create another test file (tests/unit/data/accel/hbk/test_aligner.py) by using the same code except that the mqtt server is mocked using pytest-mock. Such tests are called unit tests and are very useful.
The same technique can be used to create unit tests for Accelerometer class.
Contributor
There was a problem hiding this comment.
The existing tests in tests/data can be moved to test/integration/data.
Contributor
|
@8ohamed I forgot to mention the following comments. Please look into them.
|
Collaborator
Author
|
Hi @prasadtalasila Thanks for the review, I have now resolved the issues with accessing data_map. I have also made unit tests now we have 100% coverage for both mqtt.py and aligner.py and for accelerometer.py we have 96% |
Contributor
|
@8ohamed The tests are failing. Please see the log below. |
Collaborator
Author
|
@prasadtalasila I had forgotten to update the output of extract() in the test. It's fixed now. |
Contributor
prasadtalasila
left a comment
prasadtalasila
left a comment
There was a problem hiding this comment.
@8ohamed Thanks for the quick updates. Please see the following comments.
Contributor
There was a problem hiding this comment.
This gives the following error.
Received message on topic cpsens/d8-3a-dd-37-d2-7e/3160-A-042_sn_999998/1/acc/raw/data
Channel: cpsens/d8-3a-dd-37-d2-7e/3160-A-042_sn_999998/1/acc/raw/data Key: 282170143, Samples: 16
Fatal Python error: _enter_buffered_busy: could not acquire lock for <_io.BufferedWriter name='<stdout>'> at interpreter shutdown, possibly due to daemon threads
Python runtime state: finalizing (tstate=0x00007ffb20bf3fe8)
Current thread 0x000071f8 (most recent call first):
<no Python frame>
Extension modules: numpy._core._multiarray_umath, _wmi, numpy.linalg._umath_linalg (total: 3)
Contributor
|
The The |
Collaborator
Author
|
@prasadtalasila I have wrote the interface, and fixed the test they are all passing now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
New PR for Aligner, because the previous one was closed after renaming it.