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

[WIP] Bugfix obs transformer #903

Merged

Conversation

vincentpierre
Copy link
Contributor

Motivation and Context

Realized there was no test for obs_transforms. This is a first attempt at doing this.
The test simply makes sure that each individual transform is able to properly modify the observation_space as well as transforming an image with said dimension.

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.

@vincentpierre vincentpierre requested a review from Skylion007 July 14, 2022 14:11
@vincentpierre vincentpierre self-assigned this Jul 14, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jul 14, 2022
@vincentpierre vincentpierre changed the title [WIP] Bugfix obs transformer 001 [WIP] Bugfix obs transformer Jul 14, 2022
@Skylion007
Copy link
Contributor

There are integration tests wrapped in the baseline trainer .py class, but I agree there should be more unitifed tests. I am also pretty sure most of the cube2equirect etc... have round trip tests as well.

@vincentpierre
Copy link
Contributor Author

@Skylion007 Can you help me figure out what the shape of the transform is supposed to be? It looks like none of the obs_transform actually modify the shape of the observation, but the observation space shape is modified by the config, which confuses me. Also, shouldn't the non-target uuids obs be removed from the observation space?

@Skylion007
Copy link
Contributor

@vincentpierre No, none of the none-target uuids should be removed. You would need a seperate deleter UUID transform to do that. They can definitely modify the shape of the observation transform. A fully integrated test can be found here:

"test_cfg_path,mode,gpu2gpu,observation_transforms",
and for the equirect sensors, they can be found here:
def test_cubemap_stiching(

@vincentpierre
Copy link
Contributor Author

@Skylion007 I saw this integrated test, but I think we need a unit test. The observation output shape is currently NOT the same as the observation space shape (see the CI error). The test only works when the config shape is the same as the observation shape and it does not work in the general case.

Copy link
Contributor

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @vincentpierre

@vincentpierre vincentpierre merged commit a53b9aa into facebookresearch:main Aug 1, 2022
dannymcy pushed a commit to dannymcy/habitat-lab that referenced this pull request Jul 8, 2024
* [Bug fix + test] Adding a test for obs_transformers

* adding the test

* making the test harder

* fixing tests by removing testing on the batching dimensions

Co-authored-by: vincentpierre <vincentpierre@users.noreply.github.com>
HHYHRHY pushed a commit to SgtVincent/EMOS that referenced this pull request Aug 31, 2024
* [Bug fix + test] Adding a test for obs_transformers

* adding the test

* making the test harder

* fixing tests by removing testing on the batching dimensions

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.

3 participants