Solving infinite recursion #1
Conversation
|
Hmm, I you're right it feels like some deduplication ought to be possible, but it's tricky because the duplicated logic is so intertwined with the non-duplicated. I tried doing smth with a general version of the function that you could pass a number of hooks into, but it quickly gets messy and I'm not convinced it's better than just having some redundancy. |
|
About self nesting, Is it a normal behaviour? I can't imagine a situation when you have to support this feature. It's not hard to detect this self nesting (cycle in graph) and maybe just notify users about it? |
|
I disagree, it's of the uttermost importance to support auto nesting for operations that are not intrinsically incompatible. |
Yes, it makes sense. I also reviews you PR and you idea should work, but we have to rewrite almost everything to pass all tests. I tried to pass test_equal for auto_nested_td, but I had to write a lot of repetitive code to make it work. If you interested look at ee40162 |
|
Cool! We can think of some sort of decorator to make the logic scalable. Might be doable if we can assess what is the unitary operation to do on the contained tensors, but maybe not always obvious. |
|
@vmoens @tcbegley I was thinking about how to create some decorator to make it easy to support self nesting. Here is some idea about it with sample. Please have a look at https://pym.dev/p/2tc6a/ |
Description
I propose a solution to the infinite recursion problem.
The idea is to keep track of what is being computed with a local function.
If the current tensordict encounters itself down the line, it register what key it is and lets that aside for the moment. Once the computation is "finished" (with all the nested tensordicts let aside) it fills the keys one at a time with what has been retrieved.
Albeit the logic is repetitive i did not find a way of recycling the code across methods (
all,clone,to_tensordict,expandetc)IMO it would be better to have sort of a decorator to handle that, but I can't really think of a way to make it work in the general case.
Any idea @Zooll or @tcbegley ?