-
Notifications
You must be signed in to change notification settings - Fork 21
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
[magnitudes] Request for adding IGN magnitudes #146
base: master
Are you sure you want to change the base?
Conversation
add_IGN_magnitudes
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: root.
|
Dear Jaime and Eduardo, thank you for the contribution. Although I haven't checked everything in detail, I don't think that it is the right way to incorporate any agency specific magnitude to the SeisComP core. I recommend to implement your magnitudes as a plugin. Furthermore it could make sense to add missing calibration factor to MLc in order to avoid code duplication by using aliases. SeisComP will allow amplitude aliases in the next major version. Adding code to SeisComP core means that we are responsible for its maintenance which is not our preferred way for such a huge contribution. I recommend the following steps:
For generic amplitudes/magnitudes also of interest for other users we might consider adding them to the core. For that we need to check all of your provided codes. It would have made sense to talk to us during development to steer your development and avoid such big PRs. |
Thanks a lot also from my side. This is a great contribution! Regarding the integration I agree with @gempa-jabe that it would be ideal to have the IGN contribution as an "addon" package. This makes it easier for novice users to first concentrate on the "core" packages, while more specialized packages such as regional magnitudes are anyway more likely to target advanced users. There are good examples for contributed packages, e.g. contrib-gns or contrib-ipgp. As GFZ we have also contributed some specialized addons like the scdlpicker package or scmert. An additional advantage of a separate package (under the SeisComP umbrella) is enhanced visibility of your code contribution. |
In MLc we are trying to incorporate configurable features which cover a wide range of amplitude and magnitude requirements and we are happy to add more feature which are currently missing for your applications. We have started this approach when we realized that the number of magnitude types was growing and started to be confusing to operators. It may the case that your requirements are identical or similar to those also required by others. So why not making them available to the community? Since your code for MLcan is very similar to MLc adding your requirements may be not be a big problem. Can you point out the amplitude specifications/difference or add your missing code the existing one for MLc and provide the original scientific references? |
Thanks all for your clarifications! We will follow your suggestions and, probably, implement it as a contributed packages the Veith-Clawson 1972 magnitude at least. The difference between mb_VC and the current mb, are the Q-factors, which we think that implement them as a variable on the rst files is a bit messy. For the mb_Lg we will try to use the "alias" as a derivative from the MLc. As @gempa-dirk points out, MLcan is quite similar to MLc. The main difference is that we aim to implement a station-specific magnitude correction factor per channel, as calculated by Mezcua & Rueda (2022) for the Canary Islands. We are unsure whether it would be better to submit a pull request to MLc to incorporate this channel-based correction, or to leave it as part of the contributed package. Again, thanks for your time and your kind words! |
Dear developers,
We would like to add the amplitude and magnitude processors of the magnitudes used in routine analysis at the National Geographic Institute in Spain to SeisComp.
We have derived the amplitude and magnitude processors from the existing ones. We have not used magnitude-aliases, because we needed a further configuration. Each magnitude have been referenced properly in their corresponding files.
We don't know if we should make the pull request in this way or we have to send it by another way.
Thank you in advance!
Kind regards,
Jaime Barco and Eduardo Suarez