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.

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

fix a bug in AR picker #2157

Merged
merged 3 commits into from May 24, 2018
Merged

fix a bug in AR picker #2157

merged 3 commits into from May 24, 2018

Conversation

megies
Copy link
Member

@megies megies commented May 23, 2018

What does this PR do?

Presumably fixes buggy AR picker C code. Fix is done by @jwassermann.

I'll open it against master for inclusion into 1.2.0 since 1.1.1 is already in release candidate stage and also because this might change results significantly (just guessing, really).

It's a bit scary that the AR picker test doesn't even need to be adapted, but hey that's life. Test was adapted/improved.

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .

CC @jwassermann

@megies megies added bug confirmed bug .signal issues related to our signal processing functionalities labels May 23, 2018
@megies megies added this to the 1.2.0 milestone May 23, 2018
@krischer
Copy link
Member

Any chance to add a regression test? @jwassermann do you have some data that originally triggered the bug?

@megies
Copy link
Member Author

megies commented May 24, 2018

Any chance to add a regression test? @jwassermann do you have some data that originally triggered the bug?

Sure we can, the change is pretty subtle though, the result is changed for the S pick only and also only slightly. In the test case from 32.2s to 32.1s or so. I'll make the test more fine-grained again, there's a strange comment in there that the result is very machine dependent and therefore it's rounding to whole seconds in the test..

Copy link
Member

@krischer krischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Under the condition that CI and also the docker tests pass. The circle CI failures are dealt with in #2158 .

@megies megies merged commit 58a3cd2 into master May 24, 2018
@megies megies deleted the fix_ar_picker branch May 24, 2018 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug confirmed bug .signal issues related to our signal processing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants