-
Notifications
You must be signed in to change notification settings - Fork 22
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
Waveform plot improvements #252
Conversation
Corrected some misspelled 'Exception()' in header.py
m_to_deg was not an existing function. Used obspy locations2degrees instead.
Added a header field for N-Np-Ns which changed depending on which type of plot is used. I used a decorator to append additional header information to the header class, which gets written on plot if present. They will be present by default, but the decorator make it easy to remove if needed.
Added a new normalization based on median amplitude. Fixed issues with station_amplitude that was not working properly. Moved normalization logic (now based on precomputed list of amplitude factor instead of internal loop that compare dat and syn).
Hi Julien, Thanks for submitting these changes. Some possible discussion questions for the next call below.
|
Thanks for the comment Ryan. I think the main point for the header is that this small change with number of stations is that it isn't distracting, it's compact, and very handy to have, so it would totally make sense to have it as default. Implementing with a decorator without modifying the main Force and MTheader classes was also a way to introduce a way to have easy changes, without having the users to completely re-define a header class. I think the header could be re-designed in terms of scripting, to allow more dynamic headers and allow more optional fields to be added, but this is beyond this simple addition. As for the normalization routine, some of the problems were that: That's where pre-computing the scaling feels more manageable and flexible. But let me rewrite the median logic as a function to clean up the code a bit before moving forward. |
Don't UPDATE: Nevermind I see they aren't as you were saying. This might be something I try to correct |
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.
Julien,
Could you please submit the trace normalization changes when ready? Could you hold off on any changes to the header and annotations, and keep any customization local for now? (sorry for any inconvenience). This would help us as we try to write an operational interface (CC-ing Jeremy Webster).
Ryan
Hi Ryan, the header features such as listing the number of stations used are not customizations -- they are features that were required over the course of peer-review on previous publications. The gist is this: if you show a subset of stations in a paper (which is almost always the case), then you need to have a text indicator to distinguish these two cases: 1) 50 stations used and 6 displayed, 2) 6 stations used and 6 displayed. This is identified here: #221 The amplitude scaling issue is a longstanding one (and pre-MTUQ), with the basic issue being that you cannot view waveforms when there is a single high-amplitude outlier. Issue here: #249 |
I understand the amplitude normalization issue, addressing this would be a very useful improvement. For the header, it sounds like two different sets of needs: readability for operational use versus archiving information for later reference. I'd argue for a readable header as the default, with additional annotations being implemented via a new Header class (either by subclassing, or by writing a new one from scratch). I'd appreciate if we could take this approach as it would reduce the burden on me and Jeremy. (And indeed, you can trade out one header for another in this way.) PS For reproducibility, there's no substitute for more detailed info archived in the JSON output files? PSS Note to self, what's up with the high-amplitudes traces getting clipped again in the waveforms plot? I thought this issue was fixed at one point? |
Changed the median normalization logic into a function, in order improve readability and reduce redundancy in the plot_waveforms1 and plot_waveforms2 functions.
Hi @rmodrak, thanks again for the comments. As for the If we want to add other options, such as stats-based (we could do percentile instead of median, for instance), the normalization logic has to happen before the final trace-by-trace plotting function is called, hence the modifications I've made overall, which uses a list of normalization values for each station. As for the clipping, it wasn't a big issue previously due to the code being limited to maximum amplitude (shouldn't be able to clip) and trace-by-trace normalization (again, shouldn't clip). The median-based normalization's purpose is to tackle and be able to visualize outliers and or account for nearby data/strong motion. Therefore, there might be very different scales. Here is a dramatic example: It seems more sensible to turn clipping back on to have something more practical to newer options. My two cents on the extra field of the header: The number of stations is quite short anyway; it doesn't affect readability in the slightest while also providing some essential info for any use (particularly, as @carltape mentioned, for any kind of communication). I haven't modified station annotations, except for fixing the degree's distance option (it wasn't working either). |
Modified normalization list building logic. Previously, it would break on calls of 'station_amplitude' and 'trace_amplitude' due to previously added changes.
A possible approach based on subclassing:
Then you could just create the header and pass it as a keyword argument to the waveform plotting function. |
Hi @rmodrak , Coming back at this topic, I think there is a misunderstanding with this item, as the addition I proposed in the pull request isn't meant to be a custom UAF thing, but a general modification to the default header. I don't think having the "N-Np-Ns : x-x-x" would be detrimental to any user and it seems like a logical tweak to the default header. Let's think at it the other way, if this was already in the header: Would it make more sense to simplify the implementation, remove the decorator and hardcode this field in the header, as with the other parameters? Cheers, |
There was a suggestion to do it by subclassing but it may not have been
very clear-- if you could let me try to illustrate further tomorrow.
…On Wed, Apr 3, 2024 at 6:43 PM thurinj ***@***.***> wrote:
Hi @rmodrak <https://github.com/rmodrak> ,
Coming back at this topic, I think there is a misunderstanding with this
item, as the addition I proposed in the pull request isn't meant to be a
custom UAF thing, but a general modification to the default header.
I don't think having the "N-Np-Ns : x-x-x" would be detrimental to any
user and it seems like a logical tweak to the default header. Let's think
at it the other way, if this was already in the header:
image.png (view on web)
<https://github.com/uafgeotools/mtuq/assets/63735192/9ee5192e-11a4-49f0-831c-bc1a3e2bfd19>
I don't think users, at large, would benefit from this field being
removed. It's a basic info that is handy to have on the figure, especially
with this low of a footprint on the figure.
Would it make more sense to simplify the implementation, remove the
decorator and hardcode this field in the header, as with the other
parameters?
Let me know what you think so I can solve the conflict that popped-in with
the last merge, and rework the implementation if needed.
Cheers,
Julien
—
Reply to this email directly, view it on GitHub
<#252 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABCGSSQAAI535TGLP2MTXB3Y3SV3NAVCNFSM6AAAAABDQHO5ZSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZVHA3TENJVGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Thanks Ryan, but wouldn't that be a "custom" header and not the de-facto one, if we do so by subclassing the main header object? |
Hi Julien, As far as expanding the default header-- the difficult thing is that currently, the examples do not pass enough information to the plotting functions. I think we would want to pass this new information in a way that's backward compatible (so the old function call continues to work) and not dependent on scope in the way that a decorator might be. |
As far as the bigger picture-- continuing to support inheritance as a way of customizing figure headers I think would be beneficial. In fact, the MomentTensor and ForceHeader already use inheritance. My impression is that a decorator is a somewhat different, possibly conflicting means to this same end? Whatever the case, there is a specific idea that I would like to run by you via a pull request, if that's alright... |
Hi Ryan, I went back and check the last updated version of the code, looking at the conflicts. The addition you've made here: class MomentTensorHeader(SourceHeader):
""" Writes moment tensor inversion summary to the top of a
matplotlib figure
"""
def __init__(self, process_bw, process_sw, misfit_bw, misfit_sw,
best_misfit_bw, best_misfit_sw, model, solver, mt, lune_dict, origin,
data_bw=None, data_sw=None, mt_grid=None, event_name=None):
if not event_name:
# YYYY-MM-DDTHH:MM:SS.??????Z
event_name = '%s' % origin.time
# trim fraction of second
event_name = event_name[:-8]
self.event_name = event_name
# required arguments
self.process_bw = process_bw
self.process_sw = process_sw
self.misfit_bw = misfit_bw
self.misfit_sw = misfit_sw
self.best_misfit_bw = best_misfit_bw
self.best_misfit_sw = best_misfit_sw
self.model = model
self.solver = solver
self.mt = mt
self.lune_dict = lune_dict
self.origin = origin
# optional arguments, reserved for possible future use
# (or for use by subclasses)
self.data_bw = data_bw
self.data_sw = data_sw
self.mt_grid = mt_grid would probably enable to add the extra field without having to actually define a new class, don't you think? The reason for me implementing the decorator in the first place was to avoid modifying the arguments for the Having the grid, is actually interesting, as it would be a good gateway for having an indicator on the grid type (i.e., having the lune coordinate be displayed as Would using these fields work for you? |
I reverted the pull request back to including only the median scaling (removing new header treatment and decorator function). The new header field will come in it's own PR.
I've reverted the changes pertaining to the header. The PR now only contains the normalization features, a typo fix in header.py and a feature that wasn't correctly implemented (degree's distance for the annotations). The header change will come in another PR with a different feature branch. (Not sure why the tests aren't passing, my branch works properly locally, and all the tests are passing locally). Should be ready to merge. |
Hi Julien, Using those keyword arguments would work, thanks for being accommodating. I think someone was requesting time discretization in the header as well, probably not urgent. We could revisit maybe in context of a new release (new header, optional misfit normalization, improved colorbar limits, etc.?). May try to take this up in mid May |
Closing and reopening the pull request should cause the test to rerun. In between, could you please squash the merge commit? |
Sure, will do. |
Implementation of wavefield normalization from PR #252. Adding new feature (median-based normalization), and fixing station_amplitude.
I've made a couple of tweaks and additions to the waveform plots. Mainly the two things I wanted to tackle were:
opening up the script a little bit for additional fields in the header. It is not yet a full customizable header, but the added decorator allows to easily add additional fields to the header without messing with all the default parameters and should open up to additions if needed down the line.
tweak the normalization options. I've moved the normalization logic out of the plotting function in order to fix the stream-per-stream normalization, and allow the addition of median based normalization. I haven't changed the default (still based on the maximum value in the dataset). Having the median normalization should help visualize the dataset when big changes in amplitudes are expected (for strong-motion data for instance).
Here is an example of maximum amplitude scaling, with the Surface waves amplitude of the first station artificially increased by a factor 10:
and here is the corresponding plot using the median amplitude scaling:
There might be some clipping when using this of normalization, but it could either be tweaked to reduce it.
Per-station/stream amplitude is also now working properly.