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

Staging Updates to Python Code from Aerospace Corp. #119

Closed
wants to merge 5 commits into from

Conversation

bhilburn
Copy link
Contributor

@bhilburn bhilburn commented Aug 29, 2020

This PR stages #106 and #116 onto SigMF/master.

I am staging these here because they are both rather large changesets, and the latter builds on the former, so I don't think it makes sense to evaluate #106 on its own at this point - especially since it was filed over a year ago.

@Teque5 - I confirmed that this staging branch correctly has your two changesets by diffing it against aerospace/public/release. The only difference is that you originally branched before I merged CODE_OF_CONDUCT.md into master (see 336fcec), and you haven't pulled since then.

Update: have now also cherry-picked #115 to this branch.

Closes #106, #116, #115.

Teque5 and others added 4 commits May 7, 2019 18:06
The following squashed commit was approved for public release by **The
Aerospace Corporation** on 2019-05-07. It is covered software release
request #SW19-0024. Commits made by the *Digital Communications
Implementation Department*.

New features
- method to read & write SigMF metadata files
- method to read available captures
- method to read samples from captures
- method to get annotations
- method to count samples
- python3 module compatibility
- examples added to README

Fixes
- python3 relative path fixes
- bug in test_validation
- bug in conftest that used old SigMF datatype (f32)
- additional test in test_sigmffile
- made compatible with docker containers
- can now write multiple annotations for same sample start index

Pylint code cleanup
- removed unused imports
- fixed indentation
- fixed snake_case and too short variables
- fixed missing error definition

Other changes
- Remaining core namespace keys were added as constants to SigMFFile class.
- Sample count is automatically determined and set as a class data member
  when a data file is present.
This squashed commit is approved for public release by The
Aerospace Corporation on 2020-08-19. Commits made by the
Communication Software Implementation Department.

This commit is an addendum to the prior PR#106 approved 2019-05-07.

New Features
* Ordered sigmf-meta so that the keys are in a more useful order upon manual inspection.
* Use cases in `README.md`.

Fixes
* Improved version handling.
@bhilburn bhilburn self-assigned this Aug 29, 2020
@bhilburn bhilburn added the code label Aug 29, 2020
@bhilburn bhilburn added this to PR Filed in v1.0.0 Release via automation Aug 29, 2020
@bhilburn bhilburn added this to the Release v0.0.2 milestone Aug 29, 2020
@bhilburn
Copy link
Contributor Author

Okay, I just cherry-picked @nathan-turner111's commit from #115 over to this branch. PR #115 and #116 shared a common history in changeset but not commits, so merging them both would have turned into a merge conflict nightmare.

@nathan-turner111 @Teque5 - This PR should now include all 3 PRs that you've filed, and merging this branch will close all three. Focusing review / edits here to organize the effort.

@bhilburn
Copy link
Contributor Author

By the way, now that I've slept, #115 would have been fine, hah. Still planning to test it on top of #106 and #116 since I'm guessing that's how its being used at Aerospace. If that causes issues, we can split it back out, and then rebase it once #106 and #116 are merged 🙂

@bhilburn
Copy link
Contributor Author

bhilburn commented Oct 2, 2020

@Teque5, @nathan-turner111 - If you're able, it would be super helpful if you could pull this staged branch and test it to make sure it works as you expect as compared to your fork. Assuming it does, we should be good. If not, we just need to resolve the discrepancies.

Tagging @dkozel, as well, as I know he has an interest in getting this done too.

@bhilburn
Copy link
Contributor Author

bhilburn commented Oct 6, 2020

Just a ping on this, @Teque5! Any chance your team can give this branch a go? I believe it should be exactly what you are using in the branch on your fork.

@nathan-turner111
Copy link
Contributor

@bhilburn - I just pulled the branch and ran the GUI. It works as expected for me. However after some testing I realized I was not able to get the GUI running with python 3.6, so at least python 3.7 will be required to use it for now. I also noticed that the newest version of numpy requires python 3.5, but the project is classified under many versions of python below 3.5. I can't speak for @Teque5 commits.

@Teque5
Copy link
Collaborator

Teque5 commented Oct 6, 2020

I will take a look at this in a few hours.

@Teque5
Copy link
Collaborator

Teque5 commented Oct 7, 2020

So all the code from our PR looks fine and works with what we have. The GUI works just fine with python 3.6.9 on my Ubuntu 18.04 machine in a virtualenv. But I do have some notes:

  • Perhaps this should be in a scripts/ folder that has a link to it through the setup.py as either a script or entry_point. It seems odd that it's in the root.
  • I think little endian should be default. it's currently set to big-endian.
  • I can't figure out how to load a sigmf file. It seems I can load an archive, but not a pair of .sigmf-data and .sigmf-meta files.
  • I'm not sure I see the point of the GUI. If I started with only a data file and wanted to create an archive this would be useful but I usually start with the gr-sigmf output and already have metadata.
  • I ran it with python3 sigmf_gui.py, but you should really just add #!/usr/bin/env python3 and set it as executable.

@bhilburn
Copy link
Contributor Author

bhilburn commented Oct 7, 2020

#106 and #116 are now merged. Going to move comments from here regarding the GUI over to #115

@bhilburn bhilburn closed this Oct 7, 2020
@jacobagilbert jacobagilbert moved this from PR Filed to Done in v1.0.0 Release Feb 2, 2022
@jacobagilbert jacobagilbert deleted the staging-aerospace-prs branch January 17, 2023 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants