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

Fix text NER evaluation #7

Merged
merged 7 commits into from Feb 16, 2022
Merged

Conversation

siddalmia
Copy link
Contributor

Following the PR in #5. I started testing the evaluation pipeline for the text NER and it seems to also be quite flaky. I have fixed most of the errors but there are some critical ones that I mention in #6.

If you could make the edits to this PR for handling the remaining todos mentioned in the issue. That would be great.

I have currently kept my previous PRs also committed to this one but happy to remove them once you merge those.

Thanks
Sid

Copy link
Contributor

@ankitapasad ankitapasad left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ankitapasad ankitapasad left a comment

Choose a reason for hiding this comment

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

LGTM

The conflicts are mostly because some of the suggested changes are already accounted for in the updated repo but on different lines. Let me know if there's any case otherwise.

@sshon-asapp @fwu-asapp

@sshon-asapp
Copy link
Collaborator

@siddalmia Given that PR #10 merged, can you update the conflict files?

@fwu-asapp
Copy link
Collaborator

Hi @siddalmia @sshon-asapp
Is there any reason for changing "datasets" to "dataset" in this PR? If there is no special reason, I would just keep it as "datasets"; Otherwise, it may affect other people who have cloned the repo and are using it.

@siddalmia
Copy link
Contributor Author

Hi @siddalmia @sshon-asapp Is there any reason for changing "datasets" to "dataset" in this PR? If there is no special reason, I would just keep it as "datasets"; Otherwise, it may affect other people who have cloned the repo and are using it.

Yeah it's actually because huggingface which is needed for your baselines has a library called datasets. I have detailed it here - #5

@fwu-asapp
Copy link
Collaborator

@siddalmia Thanks for the clarification, nice catch! Do you mind to change it to data instead? Just to avoid people accidentally typing "dataset" into "datasets" or vice versa.
@sshon-asapp what do you think?

@sshon-asapp
Copy link
Collaborator

Good idea. Let move away from dataset and datasets, and use data.

@fwu-asapp
Copy link
Collaborator

I renamed "datasets" to "data" in this PR #14. @siddalmia would you please resolve the conflicts? We can merge your PR once there are no conflicts left.

@siddalmia
Copy link
Contributor Author

siddalmia commented Feb 16, 2022

@fwu-asapp I will do it soon.

But this is really bad practice. The PRs that were merged before this one were all working out of this PR. So ideally this PR should have been merged before in order to avoid merge conflicts.

@sshon-asapp
Copy link
Collaborator

sshon-asapp commented Feb 16, 2022

@siddalmia I feel sorry about this. As you may know, this toolkit is in a very early state right now, and the original authors are fixing it as quickly as possible to provide a better experience. Thank you for you understanding.

@siddalmia
Copy link
Contributor Author

@sshon-asapp @fwu-asapp I have merged this PR to the latest commit.

@siddalmia siddalmia changed the title [WIP] Fix text NER evaluation Fix text NER evaluation Feb 16, 2022
@fwu-asapp
Copy link
Collaborator

@siddalmia Thanks!
@ankitapasad and @sshon-asapp If there is not additional concerns, we can merge this branch.

@ankitapasad
Copy link
Contributor

LGTM

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.

None yet

4 participants