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

Add recursion guard and tidy tests #220

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

tcbegley
Copy link
Contributor

Description

This PR adds a new decorator _recursion_guard which catches recursion errors in methods that don't currently support auto-nesting and adds a more instructive error message.

I've removed skips from a number of methods that are in fact working. I've also removed some skips from tests which were failing for reasons that should be fixed rather than because they were testing for unsupported behaviour. Reasons for failures are listed below. I did this because otherwise I think we could trick ourselves into thinking that #201 is ready to be merged into main prematurely.

Reasons for failing tests

The following tests fail because assert_allclose_td does not support auto-nested values

  • test_from_empty
  • test_masking
  • test_getitem_ellipsis
  • test_getitem_range

The following tests fail because we can't instantiate a TensorDict from a Python dict with auto-nested values

  • test_broadcast
  • test_equal_dict
  • test_nested_dict_init

Finally test_nestedtensor_stack is failing because LazyStackedTensorDict.contiguous is broken. I think a few other methods for LazyStackedTensorDict could be broken but not caught by the tests. The issues here largely stem from the fact that values are computed lazily, and hence use of id to check for repeated values is brittle.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 14, 2023
@tcbegley tcbegley merged commit f0eede7 into auto-nested-tensordicts Feb 15, 2023
@tcbegley tcbegley deleted the auto-nested-tensordicts-tidy branch February 15, 2023 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants