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

Removing flaky by running the test only on RGB #1134

Merged
merged 3 commits into from
Feb 17, 2023

Conversation

vincentpierre
Copy link
Contributor

@vincentpierre vincentpierre commented Feb 14, 2023

Testing depth gets 1.0 values too often which makes the test fail regularly. The same functionality is achieved by running only RGB in this test.

Motivation and Context

How Has This Been Tested

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Testing RGB gets 1.0 values too often which makes the test fail regularly.
The same functionality is achieved by running only RGB in this test.
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Feb 14, 2023
@vincentpierre vincentpierre self-assigned this Feb 14, 2023
@aclegg3
Copy link
Contributor

aclegg3 commented Feb 14, 2023

Looks like this is asserting that no pixel obs(x,y) of the reset state image is the same as pixel new_obs(x,y) of an image rendered after moving the agent. Does that sound correct?

Your hypothesis is that depth (as a single channel) is more likely than RGB (with 3 channels) to trigger the test failure stochastically?

CI still seems to fail here and this assertion seems doomed to fail stochastically anyway. Maybe better to check global image similarity instead of per-pixel similarity, right? See this sim test.

@vincentpierre
Copy link
Contributor Author

Sorry, sorry. I got confused with my if statement. I was actually putting depth only in the test rather than rgb only. It should be fixed in this commit.
My guess is that depth has a high chance of returning all 1.0 because the depth maximum distance is not that high. This means that I believe that there are multiple POV from which the depth image will be all 1.0 (everything is far from the camera)

I agree that there is still some stochasticity with RGB only BUT much much less. I have the test running in a while True loop and it has not failed yet.

Copy link
Contributor

@mathfac mathfac left a comment

Choose a reason for hiding this comment

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

Thank you for fixing the test. That makes sense to run the test only for one type of modalities.
Can you elaborate more on "Testing RGB gets 1.0 values too often" ?
Which values do you mean and how RGB contributes to failures?

@vincentpierre
Copy link
Contributor Author

Can you elaborate more on "Testing RGB gets 1.0 values too often" ?

Sorry, I meant depth. It seems that the max depth of 1 is happening very often.

@vincentpierre vincentpierre merged commit cfc0fee into facebookresearch:main Feb 17, 2023
dannymcy pushed a commit to dannymcy/habitat-lab that referenced this pull request Jul 8, 2024
* Removing flaky by running the test only on RGB
Testing RGB gets 1.0 values too often which makes the test fail regularly.
The same functionality is achieved by running only RGB in this test.

* wrong condition fix

---------

Co-authored-by: vincentpierre <vincentpierre@users.noreply.github.com>
HHYHRHY pushed a commit to SgtVincent/habitat-lab that referenced this pull request Aug 31, 2024
* Removing flaky by running the test only on RGB
Testing RGB gets 1.0 values too often which makes the test fail regularly.
The same functionality is achieved by running only RGB in this test.

* wrong condition fix

---------

Co-authored-by: vincentpierre <vincentpierre@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants