-
Notifications
You must be signed in to change notification settings - Fork 32
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
Add synthetic example and documentation #168
base: master
Are you sure you want to change the base?
Conversation
Address an issue arising with the release of Matplotlib 3.8, wherein the syntax for defining shared axes was changed. Every instance of `get_shared_x_axes`/`get_shared_y_axes` has been remapped to `ax.sharex` or `ax.sharey`. This addresses Issue #161.
Add tmate to test workflow. On failure, a tmate session will be generated that can be ssh'd into to perform a debug. This is being added to address the current issue causing our tests to fail wherein there appears to be a slight numerical issue with the picked times for the Iceland icequake example.
Bypass the pick file benchmark tests for the volcano-tectonic example, for now, on Python 3.9 and 3.11 platforms. There appears to be some issue with this on the GitHub workflow platform, where a single pick differs in time in the 6th decimal place (1 microsecond). The source of this is currently unknown, but its existence means the regularly scheduled tests are not able to highlight other breaking changes in an effective manner. The picking is still being validated with the Iceland icequake benchmarks.
Skip just the event causing the test failure. This event has the EventID "20140824000443240". By just skipping this single event pick file, we remain sensitive to errors that propagate into changes to the pick times for both examples.
Switch from Black to Ruff for code formatting. Further streamlines the tooling around the project. I also suspect there will be even more tooling for Python coming from this project (e.g. unit testing). One to watch for sure. The switch caused zero lines of change in the `quakemigrate` package. I also applied the formatter to the examples scripts and the current test scripts, which had not been formatted. The pre-commit hook was updated to use `ruff format` instead of `black`.
Add a new example, which uses synthetic waveforms. This example is the one used in the manuscript to demonstrate the different stages of QuakeMigrate. The example directory contains a small module for the waveform simulation. The functions in this module use type-hinting, with the docstrings differing slightly from the standard used elsewhere in the package. A future chore might be to type-hint and update docstrings throughout the package. The example has been tested on Python versions 3.8 and 3.11. This example paves the way for a comprehensive section in the online documentation.
Add documentation covering the synthetic example added in the previous commit. This provides a commentary for the scripts, and should help to expand somewhat on why certain parameter choices are made. There is some room for improvement/extension/additions to the text, but I think it makes a good start in the current form and should be merged with only a brief pass to catch any critical issues (they were very much written as stream-of-consciousness, so could benefit from a little tightening up!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sweet. Awesome.
I've speed-read through and noticed a couple of very minor typos. Only other comment is I think adding a subsection in detect labouring the point that the LUT determines the search grid (in space) - as well as the timespan etc that you already cover - would be worth doing.
The ability to use this framework to play around with other waveforms / network configs / etc etc etc is super powerful. Nice.
*EDIT: also yes - think it doesn't harm to add as a benchmark. Could be expanded in future to provide a simple test (or basis for unit tests) of different onset functions etc etc.
Yeah, having this form a base dataset for testing different onset functions was one of the things I thought getting this integrated would be super useful for. The other one I realised out of the blue last week was—with this we might be able to sweep over STA and LTA values for a range of synthetics with different dominant frequencies, and get some concrete sense of sensible STA windows given a frequency range of interest, and how many times longer the LTA should be? |
What is the purpose of this Pull Request?
Add the synthetic example used in the QuakeMigrate paper to the examples directory and add online documentation describing each stage.
Relevant Issues
There are no relevant Issues, this is a general addition that expands on the available documentation.
Test cases
I have built the documentation locally using sphinx-build, which should hopefully replicate the building of the documentation over on readthedocs. The example itself runs (though the base branch from which I have branched off was not master but a separate branch in which some recently identified bugs had been fixed).
The example could be added as a further benchmark—thoughts? It doesn't really add too much different to the existing examples that are run to produce the benchmarks, but it equally couldn't hurt.
System details
This example was written and tested using Python 3.8 and 3.11, on macOS 12.5.1 running on a MacBook Pro with M1 chip.
Final checklist
master
(if a bugfix/patch update)version/vX.X.X
if a new significant feature (discuss with developers)