Polarization Analysis issues

[sjohnson5]

I've found a couple problems with obspy.signal.polarization.polarization_analysis

1. polarization_analysis requires parameter frqlow and frqhigh (regardless of method), however looking in the source code those parameters are only used if method="vidale". Reading the documentation I expected these to filter the stream I pass the function. (Also I feel like these parameters should be named freqmin and freqmax in order to match the names parameters that would be passed to a bandpass filter, for example)
2. On lines 508-513 of the source code (obspy/signal/polarization.py):

            if "Z" in tr.stats.channel:
                z = dat.copy()
            if "N" in tr.stats.channel:
                n = dat.copy()
            if "E" in tr.stats.channel:
                e = dat.copy()

I've got a channel named "ENZ", this channel will get all mixed up because all of the above instances are true (other channels like "ELZ" or "HNE" would also get mixed up). I would recommend replacing this with:

            if tr.stats.channel[-1] == "Z":
                z = dat.copy()
            elif tr.stats.channel[-1] == "N":
                n = dat.copy()
            elif tr.stats.channel[-1] == "E":
                e = dat.copy()
            else:
                raise NameError

Version info:

• ObsPy 1.1.0 (installed from anaconda)
• Python 3.6.6
• Ubuntu 18.04.2

Thanks for all you guys do!

[krischer]

@jwassermann this one is for you!

[megies]

polarization_analysis requires parameter frqlow and frqhigh (regardless of method), however looking in the source code those parameters are only used if method="vidale". Reading the documentation I expected these to filter the stream I pass the function. (Also I feel like these parameters should be named freqmin and freqmax in order to match the names parameters that would be passed to a bandpass filter, for example)

Yes, documentation should be clear that these are not even used with 2/3 methods, not even the default one. It's really unfortunate that these were made positional arguments, they should rather be kwargs but we can't change this without major backwards compatibility workarounds now..

I've got a channel named "ENZ", this channel will get all mixed up because all of the above instances are true (other channels like "ELZ" or "HNE" would also get mixed up). I would recommend replacing this with:

I agree, this definitly needs to be changed like proposed, but the error raised should be a ValueError.

[megies]

Also the _get_s_point function should be removed and instead reuse Stream._trim_common_channels and these routines could use some cleaning up but that's likely major refactoring and I'm not gonna touch more than necessary right now.

[megies]

@sjohnson5 please check out #2403

[sjohnson5]

Looks great! It's a bummer that frqlow and frqhigh can't be made kwargs, but I understand why.

[megies]

Looks great!

"Looks great!" as in "code comparison looks cool with all the green/red lines" or as in "tried it and works"? ;-)

[megies]

Waiting for final OP feedback

[megies]

Well closing I guess