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

ENH: signal: remezord implementation #4122

Closed
wants to merge 1 commit into from

Conversation

nils-werner
Copy link
Contributor

Rebased remezord implementation as mentioned in #1002

@zaazbb
Copy link

zaazbb commented Dec 6, 2014

I think, remezord() should have a Stopband Attenuation param, to input a Stopband Attenuation (dB) list.

firpmord() in mathlab, can input Passband Ripple (dB) and Stopband Attenuation (dB) both.
[n,fo,ao,w] = firpmord(f,a,dev,fs)
dev is a vector the same size as a that specifies the maximum allowable deviation or ripples between the frequency response and the desired amplitude of the output filter for each band.

is the rips param in remezord() equal to the dev param in firpmord()?

@zaazbb
Copy link

zaazbb commented Dec 6, 2014

Can I rewrite this matlab code to python? as below:

[n,fo,ao,w] = firpmord(f,a,dev,fs);
b = firpm(n,fo,ao,w);

change to:

[n,fo,ao,w] = remezord(f,a,dev,fs);
b = remez(n,fo,ao,w, fs);

@rgommers rgommers added scipy.signal enhancement A new feature or improvement labels Dec 7, 2014
@rgommers
Copy link
Member

rgommers commented Dec 7, 2014

There is still an unresolved discussion about whether ripples are defined correctly. Other things that need to be addressed:

  • code needs unit tests.
  • all functions except remezord need to start with an underscore.
  • the docstrings for remlplen_* repeat the same text 3 times. The 2nd and 3rd docstring can refer to the first one. The references to papers should all be moved to the remezord docstring.
  • the code needs some PEP8 cleanup (spaces around operators etc.)
  • the Hz parameter is missing from the docstring.
  • docstring format: See should be See Also, typo in Paramters, remove blank lines right below section headings.
  • An example in the docstring would be very useful.
  • remezord should be added to the function listing in signal/__init__.py.

@zaazbb
Copy link

zaazbb commented Dec 8, 2014

yes, it seems have some problems.
If i input frequency in Hz directly, this code can not work correctly, the plot output is nothing.

    fs = 1200000 # 1200 KHz.
    # bandpass, 269 KHz - 392 KHz.
    fstop1 = 269000 - 6780
    fpass1 = 269000
    fpass2 = 392000
    fstop2 = 392000 + 6780
    dstop1 = 0.002 # First Stopband Attenuation (dB)
    dpass = 0.019 # Passband Ripple (dB)
    dstop2 = 0.002 # Density Factor
    dens = 20
    N, Fo, Ao, W = remezord([fstop1, fpass1, fpass2, fstop2],
                            [0, 1, 0], [dstop1, dpass, dstop2], fs)
    bpf = remez(N, Fo, Ao, W, fs, grid_density = 20)
    w, h = freqz(bpf)
    plt.plot(w * fs / 2 / np.pi, 20 * np.log10(abs(h)))
    plt.ylabel('Amplitude [dB]')
    plt.xlabel('Frequency [Hz]')
    plt.grid()
    plt.show()

and if i input param as firpmord() in matlab, the output frequency is half of except. as below:

    fs = 1200000 # 1200 KHz.
    # bandpass, 269 KHz - 392 KHz.
    fstop1 = 269000 - 6780
    fpass1 = 269000
    fpass2 = 392000
    fstop2 = 392000 + 6780
    dstop1 = 0.002 # First Stopband Attenuation (dB)
    dpass = 0.019 # Passband Ripple (dB)
    dstop2 = 0.002 # Density Factor
    dens = 20
    N, Fo, Ao, W = remezord([i/fs/2 for i in [fstop1, fpass1, fpass2, fstop2]],
                            [0, 1, 0], [dstop1, dpass, dstop2])
    bpf = remez(N, Fo, Ao, W, grid_density = 20)
    w, h = freqz(bpf)
    # the plot result is half of (269 KHz - 392 KHz).
    plt.plot(w * fs / 2 / np.pi, 20 * np.log10(abs(h)))
    # use this line, the plot result is correct.
    #  plt.plot(w * fs / np.pi, 20 * np.log10(abs(h)))
    plt.ylabel('Amplitude [dB]')
    plt.xlabel('Frequency [Hz]')
    plt.grid()
    plt.show()

@zaazbb
Copy link

zaazbb commented Dec 8, 2014

and i need this funciton now, wich function can replace it?

@rgommers
Copy link
Member

@zaazbb I'm not sure, I'm not aware of a remezord implementation in another Python package.

@rgommers
Copy link
Member

@charris IIRC you know this stuff quite well and still have some remez improvements lying around. Apart from the things I pointed out above, what do you think about this remezord implementation?

@charris
Copy link
Member

charris commented Dec 13, 2014

I'll take a look at it. Know quite well means I studied it a bit some years ago )

@larsoner
Copy link
Member

@nils-werner are you interested in coming back to this? It looks like you force pushed an update four years ago that nobody saw (GitHub does not notify for force push updates). If the comments above are addressed and you're still interested in working on this we can review. If not perhaps we should close?

@nils-werner
Copy link
Contributor Author

Yes, I would be interested to give this another go.

@pv pv added needs-work Items that are pending response from the author and removed needs-work Items that are pending response from the author labels Aug 7, 2019
@lucascolley lucascolley changed the title remezord implementation ENH: signal: remezord implementation Mar 14, 2024
@luxedo
Copy link
Contributor

luxedo commented Jun 18, 2024

Any updates on this? I can give it a try since it looks like mostly bureaucracy.

  • code needs unit tests.
  • all functions except remezord need to start with an underscore.
  • the docstrings for remlplen_* repeat the same text 3 times. The 2nd and 3rd docstring can refer to the first one. The references to papers should all be moved to the remezord docstring.
  • the code needs some PEP8 cleanup (spaces around operators etc.)
  • the Hz parameter is missing from the docstring.
  • docstring format: See should be See Also, typo in Paramters, remove blank lines right below section headings.
  • An example in the docstring would be very useful.
  • remezord should be added to the function listing in signal/init.py.

@rgommers
Copy link
Member

I think that would indeed be nice, thanks @luxedo.

@luxedo luxedo mentioned this pull request Jun 18, 2024
9 tasks
@j-bowhay
Copy link
Member

continued in #20983

@j-bowhay j-bowhay closed this Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants