search

aonotas / deep-crf

An implementation of Conditional Random Fields (CRFs) with Deep Learning Method
http://deep-crf.com
MIT License
167 stars 48 forks source link

Support for Chainer 2.0 #2

Closed harsha-surukam closed 6 years ago

harsha-surukam commented 7 years ago

There are issues running the current build, due to code-breaking changes introduced in Chainer version 2.0. We should specify the chainer version during the installtion process as 1.24.0 or make changes to the existing code to support v2.

@aonotas, Let me know if you need me to send a pull request.

himkt commented 7 years ago

hi @harsha-surukam and @aonotas, I have edited deep-crf for chainer v2 support last week. so I make the pull request. please tell me if there are something wrong... (I am newbie to chainer.

harsha-surukam commented 7 years ago

@himkt I guess, you have only commented out the problematic parts. This definitely works, but the better solution imho would be to upgrade from v1 to v2 according to the guide.

himkt commented 7 years ago

@harsha-surukam thanks for reply 😃 I understand that it needs to follow the upgrade guide, I close the pull request.

aonotas commented 7 years ago

@harsha-surukam Thank you for your comments! If you have fixed deep-crf for v2 support, please send me PR.

aonotas commented 7 years ago

I think we need fix deep-crf code following points:

Should we switch code according to chainer.__version__?

harsha-surukam commented 7 years ago

Yes, I think that should help identify the installed version of chainer. But, I'm not sure if it's a good idea to have two branches in the code, based on the version of a library. Is it the best practice?

Or, should we just enforce people to upgrade chainer? Because, currently the default chainer version installed is 2.0.

aonotas commented 7 years ago

I asked Chainer developer just now. He told me that we need to branch code based on chainer.__version__.

harsha-surukam commented 7 years ago

Cool. Let me send you a PR in a bit.

aonotas commented 7 years ago

Thank you for your help!!

aonotas commented 7 years ago

Hi @harsha-surukam

Now deep-crf can run on v2.0 and v1.24.0!