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

PPSD: skip_on_gaps=True & subpplot spectrogram option #3220

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FMassin
Copy link
Contributor

@FMassin FMassin commented Nov 16, 2022

What does this PR do?

Why was it initiated? Any relevant Issues?

The default gap interpolation would produce ~-3000 db values, affecting plots and interpretation.

PR Checklist

  • [X ] Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • While the PR is still work-in-progress, the no_ci label can be added to skip CI builds
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just add the build_docs tag to this PR.
    Docs will be served at docs.obspy.org/pr/{branch_name} (do not use master branch).
    Please post a link to the relevant piece of documentation.
  • If all tests including network modules (e.g. clients.fdsn) should be tested for the PR,
    just add the test_network tag to this PR.
  • All tests still pass.
  • Any new features or fixed regressions are covered via new tests.
  • Any new or changed features are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .
  • If the changes affect any plotting functions you have checked that the plots
    from all the CI builds look correct. Add the "upload_plots" tag so that plotting
    outputs are attached as artifacts.
  • New modules, add the module to CODEOWNERS with your github handle
  • Add the ready for review tag when you are ready for the PR to be reviewed.

@FMassin FMassin force-pushed the PPSD-gaps-skipped-and-subplot-spec branch 3 times, most recently from 6e295f2 to 764fe0e Compare November 16, 2022 11:13
@FMassin FMassin force-pushed the PPSD-gaps-skipped-and-subplot-spec branch from 764fe0e to 29f6127 Compare November 16, 2022 11:26
@megies
Copy link
Member

megies commented Nov 16, 2022

Hmm, I'm not sure about the first part, switching the default for gaps. I'd rather keep it as is, since this is how PQLX did it and how many people are used to it. And I think most people are interested mostly in the classic Amplitude vs. Frequency, binned histogram plot, where the gappy PSD slices show up as erratic lines and can still be interpreted. Is it a big issue if you just set that option for your purposes if you know your data is very gappy?

Regarding the second part, yes we can certainly facilitate plotting into existing Axes. The "flip" option adds a lot of complexity to the code though.. do we really need that? It seems like a very rare edge case that somebody would want time on the y-Axis. Even your own example has time on x-Axis. Do we really need this "flip" option that adds so much complexity to the code?

@megies megies added .imaging Issues with our plotting functionalities .signal issues related to our signal processing functionalities labels Nov 16, 2022
@megies megies self-assigned this Nov 16, 2022
@megies megies changed the title skip_on_gaps=True & subpplot spectrogram option PPSD: skip_on_gaps=True & subpplot spectrogram option Nov 16, 2022
@FMassin
Copy link
Contributor Author

FMassin commented Nov 16, 2022

Hello

Thanks for taking care of this. I am not a PQLX user, but I have a lot of data with gaps (some for good reason, some not), the idea of interpolating gaps with zeros (the current default behaviour of obspy PPSD) never seems like a good idea to my honest opinion. I would at least add a very visible warning in obspy documentation and on the plot.

Amplitude vs. Frequency, binned histogram plot, where the gappy PSD slices show up as erratic lines and can still be interpreted

Well these erratic lines are purely an artefact of the current default: right now the code silently interprets no data as amplitude zero and sets the related PSD values to ~-3000db. There is no warning telling the user how to interpret the resulting plot.

Edit: to be more precise, the current default shows this when it is really this. In many ways, the current default looks wrong, ambiguous, and not interpretable to me. At least the median PSD is not correct, since is polluted by zeros which aren't in the data.

Even your own example has time on x-Axis

Sorry, here's the missing example

Do we really need this "flip" option that adds so much complexity to the code?

This way, it's easier to compare with an helicorder e.g. corresponding to the above.

@megies
Copy link
Member

megies commented Nov 16, 2022

the idea of interpolating gaps with zeros (the current default behaviour of obspy PPSD) never seems like a good idea to my honest opinion. I would at least add a very visible warning in obspy documentation and on the plot.

It is not a good idea in terms of meaningful signal processing, fully agree. It is meaningful in a different way, though, as it visually displays problematic features in the data, see BSSA, Vol. 94, No. 4, pp. 1517–1527 "Ambient Noise Levels in the Continental United States" by Daniel E. McNamara and Raymond P. Buland.

right now the code silently interprets no data as amplitude zero and sets the related PSD values to ~-3000db. There is no warning telling the user how to interpret the resulting plot.

Hmm need to look at that, not sure how that would happen since when adding data to PPSD it goes through the traces and cuts out slices, but I dont think it should do that when there is no data at all. It should just fill up slices when there is some data to start with.

This way, it's easier to compare with an helicorder e.g. corresponding to the above.

Hmm yea, I guess we can make do with the added complexity..

@FMassin
Copy link
Contributor Author

FMassin commented Nov 16, 2022

not sure how that would happen

... merging with 0 filling is done with method 0 by default, which means no interpolation, just filling with 0.

@megies
Copy link
Member

megies commented Nov 17, 2022

... merging with 0 filling is done with method 0 by default, which means no interpolation, just filling with 0.

Ah, I see, that's really long gaps, longer than one individual PSD slice.

@FMassin
Copy link
Contributor Author

FMassin commented Jan 6, 2023

Hey, happy new year dear @megies and ObsPy dev team :)

I really like to help with this and to see some improvements there. Please let me know if there's anything I could do or reconsider to make this compliant with your expectations.

Cheers

Fred

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.imaging Issues with our plotting functionalities .signal issues related to our signal processing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants