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

Incorrect filter design for filter gain removal in mag_calc.py #376

Closed
TomWinder opened this issue Apr 6, 2020 · 4 comments · Fixed by #378
Closed

Incorrect filter design for filter gain removal in mag_calc.py #376

TomWinder opened this issue Apr 6, 2020 · 4 comments · Fixed by #378
Labels
bug utils.mag_calc Associated with magnitude calculation issues
Milestone

Comments

@TomWinder
Copy link

When correcting for gain of pre_filt in mag_calc, a digital filter design is used rather than analog, which is not appropriate to look up the filter gain for a given frequency.

In ObsPy the iirfilter designed to apply a butterworth bandpass filter is digital, so that it is suitable to be applied using scipy.signal.sosfilt().

To look up the filter gain for a given frequency (and correct the amplitude observation for this) an analog filter should be used (as expected by the ObsPy response functions).

Current code:

z, p, k = iirfilter(corners, [lowcut / (0.5 * tr.stats.sampling_rate),
                           highcut / (0.5 * tr.stats.sampling_rate)],
                           btype='band', ftype='butter', output='zpk')
filt_paz = {'poles': list(p), 'zeros': list(z), 'gain': k,
                  'sensitivity': 1.0}
amplitude /= (paz_2_amplitude_value_of_freq_resp(
                    filt_paz, 1 / period) * filt_paz['sensitivity'])

Correct filter design:

z, p, k = iirfilter(corners, [lowcut * 2 * np.pi, highcut * 2 * np.pi],    # For analog filter, Wn in rad/s
                          btype='band', ftype='butter', output='zpk', analog=True)

See https://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.iirfilter.html

@calum-chamberlain
Copy link
Member

calum-chamberlain commented Apr 6, 2020

Morning, thanks for looking at this. It looks like you are correct - apologies for this, I clearly didn't look into paz_2_amplitude_value_of_freq_resp properly.
Is the analog equivalent actually equivalent, or should we be correcting using something like:

from scipy.signal import iirfilter, sosfreqz

sos = iirfilter(corners, [lowcut / (0.5 * tr.stats.sampling_rate),
                                   highcut / (0.5 * tr.stats.sampling_rate)],
                     btype='band', ftype='butter', output='sos')
_, gain = sosfreqz(sos, worN=[1 / period], fs=tr.stats.sampling_rate)
amplitude /= np.abs(gain)

@calum-chamberlain calum-chamberlain added bug utils.mag_calc Associated with magnitude calculation issues labels Apr 6, 2020
@calum-chamberlain calum-chamberlain added this to the 0.4.1 milestone Apr 6, 2020
calum-chamberlain added a commit that referenced this issue Apr 7, 2020
- Rewrote amp_pick_event for simplicity and clarity
- Added tests for accuracy
- Implemented correct filter gain look-up for the digital SOS
  filter applied.
@calum-chamberlain calum-chamberlain linked a pull request Apr 7, 2020 that will close this issue
6 tasks
@calum-chamberlain
Copy link
Member

@twinder36 I had a go at fixing this using the method I suggested above using the digital filter in the PR here: #378

It would be great to get your feedback on that.

@TomWinder
Copy link
Author

No worries @calum-chamberlain, I've been going through all of this with a fine tooth-comb the past few days to finalise the magnitudes module of QuakeMigrate - the magnitudes code in EQcorrscan has provided a useful reference!

Having played with the two options a bit it seems there is generally a negligible difference between using the equivalent analog response and reading it from sosfreqz, but yours is the more correct approach. Looks good to me.

@calum-chamberlain
Copy link
Member

Great, I'm glad it hasn't actually screwed up a workflow - thanks for reporting the issue!

QuakeMigrate looks like its coming along well - what more are you guys planning with it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug utils.mag_calc Associated with magnitude calculation issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants