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

"transcribed from official implementation" -> "Inspired by official implementation" in README.md #294

Closed
BrunoKM opened this issue Feb 21, 2024 · 3 comments

Comments

@BrunoKM
Copy link

BrunoKM commented Feb 21, 2024

The README.md currently says that the codebase is transcribed from the original codebase:

This implementation was transcribed from the official Tensorflow version <a href="https://github.com/hojonathanho/diffusion">here</a>

This might be too strong of a claim, as there are too many architectural differences between the two implementations (differences in where attention is placed, different number of residual streams, different number of ResNet blocks in down vs up stages, missing dropout, different activation functions in places, to name a few). It might also be somewhat misleading, as one might expect to use this repository to reproduce the results from the original paper, but the architectures are substantially different (see #114, #192).

I would suggest to rephrase the README.md as to better convey that this is a working reimplementation with differences to the original codebase. For example, "inspired by the official implementation", might be a good way to convey this. Alternatively, the codebase could be aligned more closely with the original implementation.

@BrunoKM BrunoKM changed the title "transcribed from official implementation" -> "Inspired by official implementation" "transcribed from official implementation" -> "Inspired by official implementation" in README.md Feb 21, 2024
@lucidrains
Copy link
Owner

lucidrains commented Feb 21, 2024

@BrunoKM yes you are correct. it initially was a straight transcription, but now deviates as a lot more research has progressed and got incorporated.

lucidrains added a commit that referenced this issue Feb 21, 2024
@BrunoKM
Copy link
Author

BrunoKM commented Feb 21, 2024

Awesome, thanks for addressing this so quickly!

@lucidrains
Copy link
Owner

sorry for the confusion, in case you were doing research on top of the original paper!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants