Closed alex-garvey closed 1 year ago
Thanks - that's a really good point (and I can imagine a lot of people missing that in their implementation). Very happy to add those tests
Just to check, what do you mean about the SERI MATS repo? (this isn't associated with ARENA; I'm using it for the current project I'm working on w/ Neel Nanda & Arthur Conmy)
I was referring to this, was not sure if needed a similar change. Fine either way. https://github.com/callummcdougall/SERI-MATS-2023-Streamlit-pages/blob/45c9ebef2cd9edaf0999ef2a7c1e179f46b178c4/transformer_lens/rs/arthur/interview/arena_backprop.py#L1722
Ah I see - I think that was just Arthur doing some interview prep, not relevant to the public material.
Going through the course now and just finished the backprop function. I realized that there were no tests to ensure that gradients were added if multiple nodes have the same parent.
To illustrate, the following code is in the solution:
If you were to replace that code with
grads[parent] = in_grad
all of the current tests pass. The new test will throw an error, because it has a graph where b is the parent of both d and e.
This comes from going through the solution and not understanding at first why that addition was there! The course is great so far, thank you!
I believe a similar change is needed in the SERI-MATS-2023-Streamlit-pages repo, I'm happy to make a pull request there as well if this is approved.