Skip to content

[scolv] add s-picker repicking - #206

Open
luca-s wants to merge 1 commit into
SeisComP:mainfrom
luca-s:main
Open

luca-s wants to merge 1 commit into
SeisComP:mainfrom
luca-s:main

Conversation

@luca-s

@luca-s luca-s commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

This PR extends the Repicking functionality in the Picker GUI, allowing users to run an S-picker relative to an existing P-pick instead of the current cursor position.

image

I initially developed this feature locally to help test new pickers I am working on, but realized it might be useful in general. Please feel free to close this PR if there is no interest in this functionality.

If there is interest in merging this, I already have the documentation ready to push on the src/base/master.

@cla-bot cla-bot Bot added the cla-signed The CLA has been signed by all contributors label Aug 27, 2026
@gempa-jabe

Copy link
Copy Markdown
Contributor

Thanks Luca. I think that this feature is of interest. I will hold it back some time because 7.4 is currently in its final phase and won't include that change. But we will check it later.

@luca-s

luca-s commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Sounds good to me, thanks.

Comment thread libs/seiscomp/gui/datamodel/pickerview.cpp Outdated
Comment thread libs/seiscomp/gui/datamodel/pickerview.cpp Outdated
Comment thread libs/seiscomp/gui/datamodel/pickerview.cpp
Comment thread libs/seiscomp/gui/datamodel/pickerview.cpp
Comment thread libs/seiscomp/gui/datamodel/pickerview.cpp Outdated
Comment thread libs/seiscomp/gui/datamodel/pickerview.cpp Outdated
Comment thread libs/seiscomp/gui/datamodel/pickerview.cpp Outdated
@gempa-jabe

Copy link
Copy Markdown
Contributor

I added initial code syntax remarks. There are more places but I think that you got the idea 😉.

@luca-s

luca-s commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Sorry, that was a bit sloppy on my side. I debugged the code quite thoroughly because I was using it extensively during the development of the pickers, but I did not pay as much attention to the coding style since I hadn't initially planned to make a PR out of it. I'll fix that now.

As a side note, in some cases I cannot use auto with SeisComP intrusive pointers, because auto ends up deducing the raw pointer type instead (clearly, that's needed to avoid exposing the intrusive pointers in the API). For example:

// with `auto`, picker is deduced as Processing::Picker*
Processing::PickerPtr picker = Processing::PickerFactory::Create(...);

or

// with `auto`, spicker is deduced as Processing::SecondaryPicker*
Processing::SecondaryPickerPtr spicker = Processing::SecondaryPickerFactory::Create(...);

Finally, I still have not been completely convinced by C++'s auto feature. 😉 It helps with refactoring, but sometimes it feels like a trade-off: less typing now, more detective work later when reading somebody else's code. 😄

@gempa-jabe

Copy link
Copy Markdown
Contributor

That auto cannot be used all the time is clear. Use it only where type deduction is obvious like iterators and where type deduction leads to the type you are after. I am convinced by that feature regarding more compact code. Sure, it can get confusing when reading someone else's code but if you are not familiar with the API, the concrete types won't help either 😉. I don't want to overuse it but there are obvious places where it just makes sense.

@luca-s

luca-s commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

It should be fixed now.

@luca-s

luca-s commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

@gempa-jabe anything else to do on my side?

@gempa-jabe

Copy link
Copy Markdown
Contributor

No, your are done for the moment 😉. I forwarded the feature for testing.

@luca-s

luca-s commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Thank you :)

As a side note, and there is no need to do anything about it - I am just reporting, I noticed that the uncertainty and polarity potentially returned by the picker are discarded by scolv. scautopick properly reports this information though.

@gempa-jabe

Copy link
Copy Markdown
Contributor

I know and we will be looking into it.

@gempa-jabe

Copy link
Copy Markdown
Contributor

OK, got it: the re-picker just sets the cursor and does not create a pick so the attributes are lost of course. We have actually two options: preserve extra attributes and set them once a pick is made or set a pick when re-picking. The former is actually quite nasty to implement as cursor could move by moving the mouse and now it is the question whether the extra attributes are still valid or not. The latter would be the way to go. Without breaking current behaviour, we could also implement shift+r for that mode.

@luca-s

luca-s commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

It's definitely tricky. I wouldn't rush into implementing it right away, but wait and see whether users actually push for it.

If future pickers become sufficiently reliable not only for phase identification, but also for polarity and uncertainty, I suspect users will prefer applying the picker to all stations at once rather than reviewing them station by station. At that point, the overall design could be reconsidered.

Just my two cents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed The CLA has been signed by all contributors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants