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.

Sign up for GitHub

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

Jump to bottom

Feature/315 flying squid #324

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Feature/315 flying squid #324

wants to merge 2 commits into from

Conversation

Yuuxii
Copy link
Collaborator

@Yuuxii Yuuxii commented Dec 17, 2021

FlyingSquid

@anasedova anasedova added enhancement New feature or request hackathon labels Dec 20, 2021
LuisaMaerz
LuisaMaerz reviewed Jan 4, 2022
View reviewed changes
Copy link
Collaborator

@LuisaMaerz LuisaMaerz left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me!
The transformation to the snorkel matrix with 0/1/-1 input is performed.
Also the case if no rule matches at all is handled (either delete the instance or assign a special label) and those instances are excluded from model fitting.
Is there an issue with the total number of LFs increasing if we include those instances with a new special label? For adversarial training it would be, but it might be irrelevant for the other models you have so far.
The flying squid part is fine. About the Knodle integration I am not 100% sure - looks fine at first glance, but I am not the expert here ;)

LuisaMaerz
LuisaMaerz reviewed Jan 4, 2022
View reviewed changes
Copy link
Collaborator

@LuisaMaerz LuisaMaerz left a comment

Choose a reason for hiding this comment

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

Will this affect knodle integration or are the functions just imported from the general utils and config?

@LuisaMaerz
Copy link
Collaborator

If code fully executes with Knodle there is no issues with the flying squid part and it can be merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LuisaMaerz LuisaMaerz LuisaMaerz left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request hackathon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Yuuxii @LuisaMaerz @anasedova