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.

Sign up for GitHub

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

Jump to bottom

update cmsis-dsp package source and use hanning api provided in PythonWrapper and C #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

update cmsis-dsp package source and use hanning api provided in PythonWrapper and C #1

wants to merge 2 commits into from

Conversation

xiongyu0523
Copy link

thanks for the great article, i want to provide a minor fix

@xiongyu0523 xiongyu0523 changed the title update cmsis-dsp package source and a minor fix update cmsis-dsp package source and use hanning api provided in PythonWrapper and C Mar 29, 2023
sandeepmistry
sandeepmistry reviewed May 12, 2023
View reviewed changes
Copy link
Member

@sandeepmistry sandeepmistry left a comment

Choose a reason for hiding this comment

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

Hi @xiongyu0523,

Thank you for making this pull request. It would be great to get the fix the for the text in.

However, to keep the Towards Data Science article in sync with the code, at this time I prefer to not use the new CMSIS-DSP functions in this repo. However, it's great to see that the arm_hanning_f32 function can simply things.

fixed_point_dsp_for_data_scientists.ipynb
@@ -1163,7 +1163,7 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"You can see the signal has a similiar shape to the one created with NumPy, however the values on the Y-axis range from -32768 to 32767 rather than 0 to 1.\n"
"You can see the signal has a similiar shape to the one created with NumPy, however the values on the Y-axis range from -32768 to 32767 rather than -1 to 1.\n"
Copy link
Member

Choose a reason for hiding this comment

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

Could you please make a separate pull request for fixing the text? Thanks.

@rahedges
Copy link

Hi @xiongyu0523,

Thank you for making this pull request. It would be great to get the fix the for the text in.

However, to keep the Towards Data Science article in sync with the code, at this time I prefer to not use the new CMSIS-DSP functions in this repo. However, it's great to see that the arm_hanning_f32 function can simply things.

The Toward Data Science article contains references to a deprecated CMSIDSP repo.

pip install git+https://github.com/ARM-software/[email protected]#egg=CMSISDSP&subdirectory=CMSIS/DSP/PythonWrapper

For a reader to reproduce the notebook, they need to use the new repo here:
https://github.com/ARM-software/CMSIS-DSP
and run the pip command in the PR.

If you prefer not to use the new Hanning window command, I could make a PR with the repo changes but leave the window creation intact.

@sandeepmistry
Copy link
Member

Hi @rahedges,

If you prefer not to use the new Hanning window command, I could make a PR with the repo changes but leave the window creation intact.

Thank you so much for offering to do this, that would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandeepmistry sandeepmistry sandeepmistry left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xiongyu0523 @rahedges @sandeepmistry