Skip to content
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

Support for Multiple Recordings in Archives #2

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

jhazentia
Copy link
Member

@jhazentia jhazentia commented Jun 15, 2023

This is a draft PR for discussion only. This is a simplified PR compared to #1. It does not include changes for collection. Tested in scos-sensor using mock signal analyzer. See below for proposed PR message to merge into the upstream repository:

This pull request is intended to add support for multiple recordings in archives. Below is a summary of the changes:

  • Added README examples for loading a SigMF archive with multiple recordings, creating a SigMF archive, and creating SigMF archives with multiple recordings
  • Modified SigMFArchive to accept multiple SigMFFiles
  • Renamed SigMFArchive name to path
  • Modified SigMFArchive to append to existing tarfile when a fileobj is passed that references an existing, open tarfile. Note that, if desired, this can be changed back to always overwrite existing file.
  • Added pretty parameter to SigMFArchive to control pretty printing of SigMF metadata in archives
  • Added name parameter to SigMFFile. SigMFArchive will use the SigMFFile name parameter to create recording parent directories/file names in archive
  • Renamed SigMFArchiveReader name to path
  • Modified SigMFArchiveReader to read archives containing multiple recordings. The __len__(), __iter__(), and __getitem__() operate on the list of SigMFFiles instead of individual SigMFFile
  • Modified SigMFMetafile dump() and dumps() methods to append newline to the JSON to make this behavior consistent throughout the code.
  • Added __eq__() method to SigMFFile for testing
  • Misc other supporting changes and docstring updates.
  • Added supporting tests

Note that this PR does not include any changes to include a collection in archive, read collection from archive, or to update gui.py.

These changes are based on my colleagues' (Doug Anderson and Todd Schumann) changes: https://github.com/ntia/sigmf/tree/multi-recording-archive.

Copy link
Member

@aromanielloNTIA aromanielloNTIA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, this is really close to ready. I have no suggestions/edits for the proposed PR message above. See a few comments to consider inline below.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
sigmf/archive.py Show resolved Hide resolved
sigmf/archivereader.py Outdated Show resolved Hide resolved
sigmf/archivereader.py Show resolved Hide resolved
sigmf/archivereader.py Outdated Show resolved Hide resolved
Copy link

@dboulware dboulware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider in-line comments. There may be reasons why they shouldn't be made or it won't work, but I think it is worth considering.

sigmf/archive.py Show resolved Hide resolved
sigmf/archive.py Show resolved Hide resolved
sigmf/sigmffile.py Show resolved Hide resolved
sigmf/sigmffile.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants