uafgeotools
Add taup_model as an optional parameter (not kewyword) to plot_beachball and plot_polarities
The subroutines plot_beachball and plot_polarities in their current form does not contain taup_model as a required parameter, or at least as an optional one with a default value. Therefore, if the user wants to specify the velocity model to use for calculating the piercing points on the beachball, this parameter has to be passed as a keyword argument by the user. Otherwise, the ak135 model will be used when these functions invoke _plot_beachball_matplotlib, _plot_beachball_pygmt, and _plot_beachball_gmt.
I think that this approach makes the MTUQ user prone to errors by having discrepancies in the polarity calculation - since this information is retrieved independently and in that case, the velocity model is a required parameter prior calling _takeoff_angle_taup-and the piercing points calculation when _takeoff_angle_taup is called again for plotting the stations and the polarities in the beachball, but in this case the velocity model is a keyword argument.
For example, MTUQ in its current form, when running Waveforms+Polarities.py, as is, the result is:
which is correct, since the model used is AK135.
However, the same code after changing the model to PREM, but leaving plot_beachball and plot_polarities as is yields an inconsistency between the piercing points (calculated by default with ak135) and the predicted polarities (calculated with PREM):
However, if taup_model model is passed by the user as a keyword argument , the polarity calculation and the beachball plots are consistent:
I propose to define taup_model directly in the public subroutines plot_beachball and plot_polarities, rather than being a keyword argument. I think this may help the users not having discrepancies between the predicted polarities and the piercing points by using different models in both processes. In my case, it took me some time realizing that taup_model could be passed as a keyword argument into plot_polarities, since this is not explicity mentioned in the docstring unlike plot_beachball. In the way I am proposing the modifications, mantains AK135 as the default model if is not specified in plot_beachball and plot_polarities, with the aim of not affecting the current examples, where this parameter is not passed to those subroutines.
Sorry for the long explanation. If you consider, that this modification is unncessary, I understand if you want to keep the taup_model parameter as a keyword argument in plot_beachball and plot_polarities. However, I would kindly ask, at least, to include this parameter in the docstring of plot_polarities for avoiding any confusion to current and future users.
Thanks.
Before deciding merging or not, I believe is more relevant to address this instalation issue: https://github.com/mtuqorg/mtuq/issues/307
Agreed, the polarity routines have not received as much attention as the others. Now is a good opportunity to improve them.
Agreed, better documenting taup_model and similar options could be helpful. While we're looking at it, larger changes such as moving certain calculations between the frontends and backends might also be useful?
If there's no further discussion, perhaps we could merge today. Then perhaps we could iterate further in another PR
Hi Ryan, Thanks. Yes. Sorry I have not discussed further this. I am preparing for SSA and probably the other contributors too, so I have not worked on this subject after submitting this PR. Cheers.
No problem, let's take up again after next week's presentation...
On Thu, Apr 10, 2025 at 4:25 PM Felix Rodriguez Cardozo < @.***> wrote:
Hi Ryan, Thanks. Yes. Sorry I have not discussed further this. I am preparing for SSA and probably the other contributors too, so I have not worked on this subject after submitting this PR. Cheers.
— Reply to this email directly, view it on GitHub https://github.com/mtuqorg/mtuq/pull/308#issuecomment-2795301972, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCGSSRBFC24B4KI6BGGO2D2Y3VWHAVCNFSM6AAAAAB2QEG252VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJVGMYDCOJXGI . You are receiving this because you commented.Message ID: @.***> SeismoFelix left a comment (mtuqorg/mtuq#308) https://github.com/mtuqorg/mtuq/pull/308#issuecomment-2795301972
Hi Ryan, Thanks. Yes. Sorry I have not discussed further this. I am preparing for SSA and probably the other contributors too, so I have not worked on this subject after submitting this PR. Cheers.
— Reply to this email directly, view it on GitHub https://github.com/mtuqorg/mtuq/pull/308#issuecomment-2795301972, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCGSSRBFC24B4KI6BGGO2D2Y3VWHAVCNFSM6AAAAAB2QEG252VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJVGMYDCOJXGI . You are receiving this because you commented.Message ID: @.***>
@rmodrak , @SeismoFelix , Do we have any modifications required / suggested before merging? Trying to re-open the discussion here so we can move forward.
Hi @thurinj , I do not have further suggestions from my side. However you have other PR approved before this submittion. So, feel free to take whathever you consider relevant from this suggestion and submit a more recent PR that follows the latest MTUQ modifications.