Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix phase association #126

Merged
merged 3 commits into from
Aug 22, 2022
Merged

Fix phase association #126

merged 3 commits into from
Aug 22, 2022

Conversation

damb
Copy link
Collaborator

@damb damb commented Aug 17, 2022 •
edited
Loading

Bugfixes:

  • Fix phase association. Actually, this is a regression introduced with a1d8ce8.
  • Fix default minimumArrivals module configuration parameter validation.

Closes #125.

@damb damb added bug Something isn't working detection Refers to detection related topics labels Aug 17, 2022
Daniel Armbruster added 3 commits August 17, 2022 21:32
@damb
BUG: Fix minimumArrivals config param validator
99b9aee 
Correctly validate the module configuration default.
@damb
STY: Start exception string with lower case letter
65c11e1
@damb
BUG: Correctly create mask
51695d6 
Note that this bug was introduced with
a1d8ce8. That is, while improving the
arrival offset validation performance wise.
@damb
Copy link
Collaborator Author

damb commented Aug 17, 2022 •
edited
Loading

@mmesim, I checked the results visually by means of scolv. Could you check on your side, as well (to make sure that things are working properly now)? Thanks.

@damb
Copy link
Collaborator Author

damb commented Aug 17, 2022

Besides, it would be good to add ex-03 to the test suite. Perhaps it would be sufficient to use 10-20min of the data. Just to avoid another regression.

@damb damb mentioned this pull request Aug 17, 2022
Closed
@mmesim
Copy link
Collaborator

mmesim commented Aug 19, 2022

@mmesim, I checked the results visually by means of scolv. Could you check on your side, as well (to make sure that >things are working properly now)? Thanks.

Checked and works fine. Thanks a lot.

PS. I checked the ex-03 with the data I sent you and it is ok. Perhaps you can add that to the test suite. 🏊

@damb
Copy link
Collaborator Author

damb commented Aug 22, 2022

Due to #129 and #130 at the time being it doesn't seem sensible to add ex-03 to the test suite. Instead I'll merge this one. The test suite should include the ex-03 test case once both #129 and #130 are fixed.

@damb damb merged commit f5ccc42 into master Aug 22, 2022
@damb damb deleted the feature/iss-125 branch August 22, 2022 14:44
@damb damb mentioned this pull request Aug 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug Something isn't working detection Refers to detection related topics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Phase association issue (regression)
2 participants
@damb @mmesim