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

More numpythonic osc funcs, now w/ more args (phase, amp, duty cycle) #23

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

jbdoar
Copy link
Collaborator

@jbdoar jbdoar commented Dec 27, 2020

Fixes # .

Changes proposed in this pull request:

Mentions: @alxrsngrtn @jbdoar

Copy link
Owner

@alxmrs alxmrs 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.

Nit: Instead of using single-line comments for low and hi pass, use a """docstring""".

@alxmrs alxmrs self-requested a review December 28, 2020 02:45
Copy link
Owner

@alxmrs alxmrs left a comment

Choose a reason for hiding this comment

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

Before merge, please make sure bin/bish still works.

@jbdoar
Copy link
Collaborator Author

jbdoar commented Dec 28, 2020

Before merge, please make sure bin/bish still works.

Good idea. Do you have any minimal working examples of how to get that part of the project to work? I haven't figured it out yet.

@alxmrs
Copy link
Owner

alxmrs commented Dec 28, 2020

Yes -- I believe examples should be documented in the help message of the script. So, running bin/bish --help will do the trick. I think you will hit import errors before then.

@jbdoar
Copy link
Collaborator Author

jbdoar commented Dec 28, 2020

Well, flattening the file structure of the hcm library certainly has introduced some errors--we certainly won't wish to merge anything yet.

@jbdoar
Copy link
Collaborator Author

jbdoar commented Dec 29, 2020

Alright, so: when I try running bish, I get this error, flagging line 39 in the io module:
AttributeError: module 'rx' has no attribute 'Observer'

Any thoughts on where that might be coming from? In case it matters, I have rx version 3.1.1 installed on my machine, which differs from the version in your requirements.txt.

@alxmrs
Copy link
Owner

alxmrs commented Dec 29, 2020

Ah. So the rx api has changed from our reqs to the latest release. The easy fix would be to uninstall rx and pip install -r requirements.txt. We also could refactor our use of rx in the project to use the latest api. The latter is a lot of effort, so I'm ok with doing the former.

@jbdoar
Copy link
Collaborator Author

jbdoar commented Jan 6, 2021

Will need more reviewing before merge. Presently I am no longer getting errors when running bish, but it's not driving my speakers, either...

@alxmrs
Copy link
Owner

alxmrs commented Jan 9, 2021

hmm... it looks like bin/bish was deleted in this branch. How should I test it?

dunno how it disappeared from the branch
@@ -11,7 +11,7 @@ from hcm import types
SAMPLE_RATE = 8000 # hz
INTERVAL_LENGTH = 1000 # ms

Copy link
Owner

Choose a reason for hiding this comment

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

I see you moved the file from bin. Please update setup.py to point to this new location of bish:

scripts=['hcm/bish']

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm sure there was a better way than moving bish into the main directory, but for whatever reason it wasn't finding hcm when I tried running it out of the bin folder.

@alxmrs
Copy link
Owner

alxmrs commented Jan 10, 2021

it's not driving my speakers

I see. I filed a bug for this, I can look into and fix it. Once you update setup.py, this PR is good to go.

@alxmrs alxmrs self-requested a review January 10, 2021 03:46
Copy link
Owner

@alxmrs alxmrs left a comment

Choose a reason for hiding this comment

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

LGTM, after minor change to setup.py.

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.

None yet

2 participants