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

initial commit on waveform timemarks featuring #2766

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

Conversation

mbagagli
Copy link

@mbagagli mbagagli commented Dec 9, 2020

What does this PR do?

Yet another feature on Stream.plot() method. This PR introduces a new boolean parameter called plot_time_marks. If set to True, this parameter will allow the user to plot timemarks on plot types normal, relative and section. These time marks are customizable like any other matplotlib line object.

These timemarks are defined in an additional attribute in Trace.stats object. The attribute is a container of tuples/lists of 2 elements each: a time, and a matplotlib dictionary plot.

tr.stats.timemarks=[
            (UTCDateTime("2011-11-24T21:11:07.0"),
            {'color': 'b', 'markeredgewidth': 3, 'markersize': 10 ...}),
            ...
]

Why was it initiated? Any relevant Issues?

It started due to a personal need to fast-plot custom TimeMarks over Stream section's plot.
Then I thought the community could also benefit from it, and I've implemented also on normal and relative type-plot as well.
We all know that users must create their own custom scripts for ad-hoc figures, but this additional parameter could give a quick vision of different time-features (ie. picks, swarns etc..) and maybe helps new python users in achieving so.
No relevant issues. At the moment it is not implemented in the dayplot type but I could of course add it if we prefer.

Documentation-update and tutorial are provided.

PR Checklist

  • 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).
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: "+DOCS"
  • If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:"
    (you can also add "ALL" to just simply run all tests across all modules)
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .

@heavelock
Copy link
Contributor

I have mixed feelings about using as input a field in tr.stats. It's not vere explicit and quite much separates data from plotting.
I was recently implementing some per trace processing and I went in direction of using as argument a dict with seedid as key which I think is a bit more clear for the user. From the perspective of plotting routines it's probably a bit more complicated though.

What are your thoughts?

@mbagagli
Copy link
Author

@heavenlock your points are right but I think it could be the classic 2-faces of the same medal.
One could also think to pass a list of time-objects directly at invocation. But while being easy to do for a single Trace object, it would became cumbersome in case of big streams (as you said we would need a seedid linked to each single trace to keep timemarks separated).

In my opinion it would be easier (with all the updated documentation) to associate timemarks directly on the trace of interests rather than using a separate dict with seedid.
I think that originally the idea of such a plot method was to quickly document the state of a Stream directly with different methods.

I've also came up with the idea of using a list of modified origin.Pick class instances, but rather than adding attributes to that, I preferred to use standard dictionaries. This choice came also from the fact that trace.stats is holding metadata, and some official tweaks has been done before (i.e. trace.attach_response() that creates the trace.stats.response attribute.

Maybe we could think of a better structured dictionary for this field? Or maybe creating a really small class Timemark? Timemarks may indeed refer to totally un-correlated information respect to the waveforms and should not be treated strictly as phase-picks. Eventually, this attribute could be used in at some stage in other class-methods as well?

@heavelock
Copy link
Contributor

I see your point here. I think the best solution for this problem would be really going for either little helper class as you are saying or some other defined data structure. This would help with validation of the inputs from users and probably helped a lot with reusability of that structure.

However, this I think need a broader discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants