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

Add directory of the collection to SigMFCollection #85

Merged
merged 15 commits into from
Jan 8, 2025

Conversation

GreenK173
Copy link
Contributor

Solves #84 by adding an optional parameter collection_path to SigMFCollection which is the path to the directory where the collection lives. Plus an unit test.

@Teque5
Copy link
Collaborator

Teque5 commented Dec 14, 2024

  1. Can you change this to use tempfile like how I did for PR#81 so it doesn't create stray files?
  2. Please rename the test to test_collection.py so we can put other tests in there.

After that I'll review in more detail.

@GreenK173
Copy link
Contributor Author

Rewritten the unit test per above comment.

@Teque5
Copy link
Collaborator

Teque5 commented Dec 20, 2024

I re-read SigMFCollections and relevant spec and I think this PR enables some functionality we may not want. The spec says:

The sigmf-collection file MUST be either in the same directory as the Recordings that it references, or in the top-level directory of an Archive (described in later section).

I believe this means that the only way the metadata is not in the same folder as the .sigmf-collection file if it is within an archive. I wouldn't want someone to define a collection within a folder that allowed the metadata to be anywhere.

Two items for @GreenK173:

  • Can we do something to check if the collection is within an archive? Would that allow us to not need collection path?
  • If you think this is fine as-is, then we should modify the test to at least only cover the case where we are using fromarchive() instead of the current fromfile().

I'm not super eager to add this new top level init key collection_path unless we really need it, but the rest of this PR is valuable either way.

@GreenK173
Copy link
Contributor Author

I re-read SigMFCollections and relevant spec and I think this PR enables some functionality we may not want. The spec says:

The sigmf-collection file MUST be either in the same directory as the Recordings that it references, or in the top-level directory of an Archive (described in later section).

I believe this means that the only way the metadata is not in the same folder as the .sigmf-collection file if it is within an archive. I wouldn't want someone to define a collection within a folder that allowed the metadata to be anywhere.

There seems to be a misunderstanding. Yes if we have a .sigmf-collection file then it needs to be in the same directory as the recordings it contains, I'm not proposing any changes to that. However a SigMFCollection instance can exist with or without any existing .sigmf-collection file, when we create it we basically give it a list of filenames of the recordings (either by metafiles or metadata parameter), but then the instance also needs to know the path to the recordings. And in the current version it has no way of knowing it (unless you want the recording filenames to be full paths, but that doesn't seem to be the indended use and would cause other issues) so it only works if the recordings are in the working directory of the script.

None of the proposed changes affect the functionality for archives (and collections in an archive are not currently implemented anyway).

@Teque5
Copy link
Collaborator

Teque5 commented Jan 7, 2025

I re-read SigMFCollections and relevant spec and I think this PR enables some functionality we may not want. The spec says:

The sigmf-collection file MUST be either in the same directory as the Recordings that it references, or in the top-level directory of an Archive (described in later section).

I believe this means that the only way the metadata is not in the same folder as the .sigmf-collection file if it is within an archive. I wouldn't want someone to define a collection within a folder that allowed the metadata to be anywhere.

There seems to be a misunderstanding. Yes if we have a .sigmf-collection file then it needs to be in the same directory as the recordings it contains, I'm not proposing any changes to that. However a SigMFCollection instance can exist with or without any existing .sigmf-collection file, when we create it we basically give it a list of filenames of the recordings (either by metafiles or metadata parameter), but then the instance also needs to know the path to the recordings. And in the current version it has no way of knowing it (unless you want the recording filenames to be full paths, but that doesn't seem to be the indended use and would cause other issues) so it only works if the recordings are in the working directory of the script.

None of the proposed changes affect the functionality for archives (and collections in an archive are not currently implemented anyway).

Okay to make this more clear, shall we change the kwarg from collection_path to base_path and the internal property path to base_path? I think that would be the final touch.

Also make that property a pathlib.Path.

@GreenK173
Copy link
Contributor Author

Okay to make this more clear, shall we change the kwarg from collection_path to base_path and the internal property path to base_path? I think that would be the final touch.

Also make that property a pathlib.Path.

Done.

@Teque5 Teque5 merged commit d88719a into sigmf:main Jan 8, 2025
3 checks passed
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.

2 participants