-
Notifications
You must be signed in to change notification settings - Fork 0
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
Multi recording archive #1
base: main
Are you sure you want to change the base?
Conversation
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.
Justin- I like the approach you took overall here. See the comments I provided for a couple suggested tweaks or things to consider. Aside from these, I don't think these changes are too big to consider opening a PR in sigmf/sigmf-python. I do think that it would make sense to add similar functionality for SigMF Collections at the same time. It might be helpful to consider the different possibilities documented here
… "name" to "path", rename duplicate test name
…n archive, add __eq__ method for SigMFCollection, improved handling of metadata in collection, fixed bug in read_samples()
… to fromarchive()
Note: no changes made to support GUI |
Recommend merging sigmf/sigmf:main into this branch before opening the PR there- looks like there are 2 commits to merge in |
…i-recording-archive
Done |
…ve usability with SigMFArchiveReader
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.
Overall, looks good- left a few minor comments/suggestions to consider below
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.
See in-line comments. I believe only one remains regarding adding a test for fromarchive with a SigMFCollection.
|
||
self._check_input() | ||
|
||
archive_name = self._get_archive_name() | ||
mode = "a" if fileobj is not None else "w" |
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.
Eventual PR should call out this change in behavior and note that it could be changed to preserver the original behavior of writing over the archive.
sigmf/sigmffile.py
Outdated
"""Dump contents to SigMF archive format. | ||
|
||
`name` and `fileobj` are passed to SigMFArchive and are defined there. | ||
`file_path` is passed to SigMFArchive `path` and `fileobj` is passed to |
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 should probably describe the handling of file_path and name
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.
Fixed in e4e1775
tests/test_archive.py
Outdated
assert file2_ext in file_extensions | ||
|
||
|
||
def test_sf_fromarchive_multirec(test_sigmffile, test_alternate_sigmffile): |
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.
I may have missed it, but is there a similar test for SigMFCollection.fromarchive?
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.
See updates in 3131683
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.
Looks good to me.
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.
Looks good!
…istency with collection stream names, also adding files directly to tar instead of using temp folder
This PR should not be merged into NTIA/sigmf-python main. This is just for discussion.
Tested on my computer using mock sigan.