Skip to content

Add support for Python 3#3

Closed
bryant1410 wants to merge 2 commits into
datquocnguyen:masterfrom
bryant1410:python3
Closed

Add support for Python 3#3
bryant1410 wants to merge 2 commits into
datquocnguyen:masterfrom
bryant1410:python3

Conversation

@bryant1410

Copy link
Copy Markdown

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

@datquocnguyen

Copy link
Copy Markdown
Owner

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

@bryant1410

Copy link
Copy Markdown
Author

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

@datquocnguyen

datquocnguyen commented Sep 21, 2018

Copy link
Copy Markdown
Owner

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
Copy Markdown
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
Copy Markdown
Owner

@bryant1410

Copy link
Copy Markdown
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
Copy Markdown
Author

you were using python 2 or 3?

@bryant1410

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

Note that you need to install the package future

@datquocnguyen

datquocnguyen commented Sep 22, 2018

Copy link
Copy Markdown
Owner

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.

2 participants