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

Change __getitem__ to handle a single item or a slice #31

Merged
merged 2 commits into from
Oct 25, 2023
Merged

Conversation

bebau
Copy link
Contributor

@bebau bebau commented Oct 22, 2023

Proposed fix for #25 .

One other difference I noticed between slicing and read_samples() that makes this slicing behavior slightly strange: a slice will not scale int8 data (or similar) to a float, while read_samples will. Maybe that's not a bug and get_item should return truly "raw" data, but it could be warning-worthy.

Copy link
Collaborator

@Teque5 Teque5 left a comment

Choose a reason for hiding this comment

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

Change set looks good. It would be great if there was a test that worked for this method of accessing memory-mapped samples.

sigmf/sigmffile.py Outdated Show resolved Hide resolved
sigmf/sigmffile.py Outdated Show resolved Hide resolved
sigmf/sigmffile.py Outdated Show resolved Hide resolved
sigmf/sigmffile.py Outdated Show resolved Hide resolved
…i-channel case.

Apply suggestions from code review and add test.

Co-authored-by: Teque5 <[email protected]>
@Teque5
Copy link
Collaborator

Teque5 commented Oct 24, 2023

This is just about perfect, but follow this pattern and use a tempfile for the path. I know there are examples in there using /tmp/whatever but that only works on some unix-like systems. I've been meaning to change those all to use the other pattern.

@Teque5
Copy link
Collaborator

Teque5 commented Oct 24, 2023

Opened #32 as a reminder to fix those testing issues.

@Teque5 Teque5 merged commit 6c90458 into sigmf:main Oct 25, 2023
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