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

Add support for Python 3 #3

Closed
wants to merge 2 commits into from

Conversation

bryant1410
Copy link

I tested converting, training and predicting in both Python 2 and 3.7 and it worked fine.

@datquocnguyen
Copy link
Owner

Many thanks. I am wondering whether it would work with the pre-trained models I released ?

@bryant1410
Copy link
Author

I tested it with "model256" and it does work.

@datquocnguyen
Copy link
Owner

datquocnguyen commented Sep 21, 2018

I reran the model with your "python3" branch on the UD Norwegian NynorskLIA corpus. It first returned a "UnicodeDecodeError". After fixing I got warnings "UnicodeWarning: Unicode equal comparison failed to convert both arguments to Unicode - interpreting them as being unequal", and then "IndexError: list index out of range" in the load_embeddings_file function. I do not have time to further support this, as I am doing a different project. So I'd to inform you that I close this pull request. Thanks!

@bryant1410
Copy link
Author

I reran the model with your "python3" branch on the UD Norwegian NynorskLIA corpus. It first returned a "UnicodeDecodeError". After fixing I got warnings "UnicodeWarning: Unicode equal comparison failed to convert both arguments to Unicode - interpreting them as being unequal", and then "IndexError: list index out of range" in the load_embeddings_file function. I do not have time to further support this, as I am doing a different project. So I'd to inform you that I close this pull request. Thanks!

I disagree with closing it even if you don't have time, you could just leave it open. But anyway, can you share the first fix you did and what you run to test it exactly? Maybe I can fix it.

@datquocnguyen
Copy link
Owner

https://drive.google.com/open?id=1d9mlsx_kmSQmwpY9I57Fmvrw6cE8rQlj The errors I got when I ran with python2.

@bryant1410
Copy link
Author

This worked fine for me (python 3):

python jPTDP.py --predict --model model256 --params model256.params --test UD_Norwegian-NynorskLIA/no_nynorsklia-ud-test.conllu --outdir output2 --output abc
python jPTDP.py --outdir output --train UD_Norwegian-NynorskLIA/no_nynorsklia-ud-train.conllu --dev UD_Norwegian-NynorskLIA/no_nynorsklia-ud-dev.conllu

I don't know what the other files are for (vectors and the XML file).

@bryant1410
Copy link
Author

you were using python 2 or 3?

@bryant1410
Copy link
Author

I pushed a fix to the branch, which I think it doesn't appear here because it's closed. The issue was that I tested before with English files, that have no accent. Now it's working both for python 2 and 3, converting training and testing. Please, test it yourself and let me know if you run into troubles.

@datquocnguyen
Copy link
Owner

Thanks. I reopen the pull request. I do not have time to test it now. But I will spend sometime to do it!

@datquocnguyen datquocnguyen reopened this Sep 21, 2018
@bryant1410
Copy link
Author

Note that you need to install the package future

@datquocnguyen
Copy link
Owner

datquocnguyen commented Sep 22, 2018

I retested with python2 (I could not test with python3 as numpy was broken with python3 on my Mac). Anyway, the major issue is that running the new "porting" code with the pre-trained models I released significantly degraded the original performance. You can verify it on the the UD Norwegian NynorskLIA corpus yourself. In other words, with the new code it would require to retrain all models, and I do not have time to redo it. So I have decided to close this pull request. Instead, I might make a link to your porting code. Thanks.

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

2 participants