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

Gyroscope Bias Estimation - Unnecessary Max Operation #462

Closed
wants to merge 1 commit into from

Conversation

AlainSchoebi
Copy link

Summary
This pull request removes an unnecessary std::max operation in the gyroscope bias estimation code. This improves code readability and efficiency without impacting the functionality of the algorithm.

Description
The targeted code block checks if the gyroscope sample's norm exceeds a threshold (kGyroscopeForBiasThreshold). If it does, the function exits early; otherwise, it computes an update weight.

Here is the code block:

  if (gyroscope_sample_norm2 >= kGyroscopeForBiasThreshold) {
    return false;
  }

  float update_weight = std::max(
      0.0f, 1.0f - gyroscope_sample_norm2 / kGyroscopeForBiasThreshold);
  update_weight *= update_weight;

For the std::max operation to pick 0.0f over 1.0f - gyroscope_sample_norm2 / kGyroscopeForBiasThreshold, we need

0 >= 1 - gyroscope_sample_norm2 / kGyroscopeForBiasThreshold

which implies that

gyroscope_sample_norm2 >= kGyroscopeForBiasThreshold

However, this cannot occur due to the previous if-statement if (gyroscope_sample_norm2 >= kGyroscopeForBiasThreshold), which would exit the function before reaching this point.

This suggests that the max operation is unnecessary here and can be omitted for code readability and efficiency.

@AlainSchoebi AlainSchoebi changed the title gyroscope bias estimator removed max operation Gyroscope Bias Estimation - Unnecessary Max Operation Apr 18, 2024
@arilow
Copy link

arilow commented Apr 22, 2024

Thanks @AlainSchoebi for your pull request! Changes LGTM.

I ticket has been created to test your changes and merge as soon as we're able to do it internally.

@AlainSchoebi
Copy link
Author

Hi,

Thanks, @arilow. Do you have any updates on this? It appears that the merge has failed.

Best regards,
Alain

@arilow arilow added the cla: yes label Aug 8, 2024
@lneumarkt
Copy link

Your proposed changes have been already internally merged and will be included in the next release. Thanks for contributing to Cardboard!

@xinyunh0929
Copy link
Contributor

This was merged in v1.26.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Targeting 1.26
Development

Successfully merging this pull request may close these issues.

4 participants