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

BestRQ implementation #63

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

BestRQ implementation #63

wants to merge 5 commits into from

Conversation

Judyxujj
Copy link
Contributor

add the two components, i.e. mask and quantiser, for BestRQ.

@Judyxujj Judyxujj self-assigned this Nov 18, 2024
@vieting vieting changed the title BestBQ implementation BestRQ implementation Nov 19, 2024
Copy link
Contributor

@michelwi michelwi left a comment

Choose a reason for hiding this comment

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

Not sure if there are problems in the code or if I misunderstood what it is doing 🤷

i6_models/parts/best_rq/mask.py Outdated Show resolved Hide resolved
i6_models/parts/best_rq/mask.py Show resolved Hide resolved
i6_models/parts/best_rq/mask.py Show resolved Hide resolved
i6_models/parts/best_rq/mask.py Show resolved Hide resolved
i6_models/parts/best_rq/mask.py Outdated Show resolved Hide resolved
i6_models/parts/best_rq/quantizer.py Show resolved Hide resolved
@albertz
Copy link
Member

albertz commented Nov 25, 2024

Does it make sense to include this in i6_models? What exactly is our policy w.r.t. what we want to have here? I thought i6_models is intended for models (and maybe other functions) which are supposed to be used by a larger amount of people among us. But it is not intended for research code, where you want to try something out. Here, as I understand, this is mostly adapted from Fairseq, so maybe it's already well tested, but I don't know. Did you test this?

Jingjing Xu and others added 2 commits December 4, 2024 14:35
@Judyxujj
Copy link
Contributor Author

Judyxujj commented Dec 4, 2024

Does it make sense to include this in i6_models? What exactly is our policy w.r.t. what we want to have here? I thought i6_models is intended for models (and maybe other functions) which are supposed to be used by a larger amount of people among us. But it is not intended for research code, where you want to try something out. Here, as I understand, this is mostly adapted from Fairseq, so maybe it's already well tested, but I don't know. Did you test this?

I think BestRQ is a very general pre-training architecture that we might use later also in the group, so I add it here. I ran them with a small dataset and seems to work.

i6_models/parts/best_rq/mask.py Show resolved Hide resolved
@curufinwe
Copy link
Contributor

Does it make sense to include this in i6_models? What exactly is our policy w.r.t. what we want to have here? I thought i6_models is intended for models (and maybe other functions) which are supposed to be used by a larger amount of people among us. But it is not intended for research code, where you want to try something out. Here, as I understand, this is mostly adapted from Fairseq, so maybe it's already well tested, but I don't know. Did you test this?

I have no problem adding also some code that is a bit more "research-y" to the repo. But of course once it's in we would require that new variants get a separate class name to not break the old behavior (bugs are the exception of course). If we see that some model / part of a model is changing rapidly we might want to delay things until it settles down a bit, but in this instance I would expect the code from fairseq to broadly work.

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

Successfully merging this pull request may close these issues.

4 participants