JuliaText / TextAnalysis.jl

Julia package for text analysis
Other
373 stars 95 forks source link

Fix Sequence Labelling Models, fixes #178 #180

Closed Ayushk4 closed 3 years ago

Ayushk4 commented 4 years ago

The new weights file for NER is here

Addresses #178

Ayushk4 commented 4 years ago

The weights modified weights for POS is here.

Ready for review. Travis tests pass for Julia 1.x

Ayushk4 commented 4 years ago

@aviks Travis tests pass for Julia 1.0, but has compatibility issue for Tracker with Julia 0.7. What do you suggest me to do.

aviks commented 4 years ago

We don't want to support 0.7. Only 1.0 and 1.3

On Wed, 18 Dec 2019, 11:36 Ayush Kaushal, notifications@github.com wrote:

@aviks https://github.com/aviks Travis tests pass for Julia 1.0, but has compatibility issue for Tracker with Julia 0.7. What do you suggest me to do.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JuliaText/TextAnalysis.jl/pull/180?email_source=notifications&email_token=AAC4QJSU66R46QIQ5UUSTXDQZG4UBA5CNFSM4J2MG3RKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHE66UA#issuecomment-566882128, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC4QJXOQYBB4QNKXCLXJYTQZG4UBANCNFSM4J2MG3RA .

Ayushk4 commented 4 years ago

So, should I send a separate PR updating travis and appveyor ~files~ scripts?

aviks commented 4 years ago

Yup

On Wed, 18 Dec 2019, 12:25 Ayush Kaushal, notifications@github.com wrote:

So, should I send a separate PR updating travis and appveyor files?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JuliaText/TextAnalysis.jl/pull/180?email_source=notifications&email_token=AAC4QJQ54IR77NBGY4FCZJDQZHCNVA5CNFSM4J2MG3RKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHFB4GI#issuecomment-566894105, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC4QJRLCCOMADQV3SFROFDQZHCNVANCNFSM4J2MG3RA .

aviks commented 4 years ago

@Ayushk4 you should be able to upload the weights to the release on this repo and update the source.

Ayushk4 commented 4 years ago

@aviks Thanks, I will do that once all the tests pass.

aviks commented 4 years ago

Do you know why we've suddenly started to get these NNPACKError(code 26, NNPACK STATUS UNSUPPORTED ALGORITHM)? I know I had tests passing locally a few days ago, but now see these on my laptop as well. What has changed? Any ideas?

Ayushk4 commented 4 years ago

I am not sure. After merging the changes from #181 to this patch, all the tests were passing (except for 32-bit appveyor). But now they seem to be failing.

This is the first time I have come across NNPACKError(code 26, NNPACK STATUS UNSUPPORTED ALGORITHM) error. I am not sure what could be the reason for this.

Ayushk4 commented 4 years ago

NNLib.jl recently had a patch release. I can try restricting the version to before that.

aviks commented 4 years ago

Yes, I was about to say, It's probably due to NNlib v0.6.1

Ayushk4 commented 4 years ago

Even after changing Project.toml, Travis still installs NNlib 0.6.1 instead of the mentioned 0.6.0. Weird.

aviks commented 4 years ago

See discussion on slack #pkg-usage :

[compat]
Flux = "< 0.10"
NNlib = "< 0.6.1"
julia = "1"
Ayushk4 commented 4 years ago

That works! Travis tests pass now, only 32-bit appveyor left to deal with.

Ayushk4 commented 4 years ago

Turns out that this is v0.6.1 is the first NNlib release since support for NNPACK pr got merged. I think the error we were getting might have something to do with that, probably

Ayushk4 commented 4 years ago

@aviks Is there any way to test Julia 32-bit on my 64-bit system? These tests solely fail on 32-bit on the bson files that were saved in a 64-bit system. I want to separate an MWE and report it and/or find a workaround.

Ayushk4 commented 4 years ago

Here's more on the 32-bit issue with BSON.jl- JuliaIO/BSON.jl#60

aviks commented 4 years ago

That is great debugging. Very cool, @Ayushk4

Ayushk4 commented 4 years ago

Thanks, I have filed a PR for the same. However, since it's holidays we could expect a delay in the review work. I believe that should fix the problems being faced in both the sequence and the sentiment models on 32-bit systems.

Ayushk4 commented 4 years ago

Mike Innes has merged the PR. Is there anyways we can test the CI with the master branch of BSON.jl?

aviks commented 4 years ago

@Ayushk4 what is left on this one?

Ayushk4 commented 4 years ago

I was waiting for JuliaIO/BSON.jl to have a new release with the fixes I made. I don't recall exactly what else is left. I will fix tomorrow.

aviks commented 4 years ago

@Ayushk4 Bump?

Ayushk4 commented 4 years ago

I will look into it this Sunday.

Ayushk4 commented 4 years ago

@aviks All models (NER, POS, sentiment and PerceptronTagger) work on 32-bit as well. 1 Test in ULMFiT seems to be failing in 1.3

Ayushk4 commented 4 years ago

ULMFiT tests are passing locally for me on 1.3.1, updated Manifest.toml.

Ayushk4 commented 3 years ago

@aviks Please review.