Add taup_model as an optional parameter (not kewyword) to plot_beachball and plot_polarities #308
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The subroutines plot_beachball and plot_polarities were hard-coded for using ak135 models when invoking _plot_beachball_matplotlib, _plot_beachball_pygmt, and _plot_beachball_gmt. Now plot_beachball and plot_polarities includes explicitly taup_model parameter, but if this parameter is not defined by the user, then the ak135 model is used by defautl.
It is better to leave taup_model as a parameter with default value of ak135, rather than being a required parameter in the private subroutines _plot_beachball_gmt, _plot_beachball_pygmt, and plot_beachball_matplotlib.
SeismoFelix
changed the title
|
Before deciding merging or not, I believe is more relevant to address this instalation issue: #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 |
|
Possibly relevant: #309 |
|
If there's no further discussion, perhaps we could merge today. Then perhaps we could iterate further in another PR |
|
Hi Ryan, |
|
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
<#308 (comment)>, 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)
<#308 (comment)>
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
<#308 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABCGSSRBFC24B4KI6BGGO2D2Y3VWHAVCNFSM6AAAAAB2QEG252VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJVGMYDCOJXGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.