kmkurn / pytorch-crf

(Linear-chain) Conditional random field in PyTorch.
https://pytorch-crf.readthedocs.io
MIT License
935 stars 151 forks source link

Refactor old #54

Closed r0mainK closed 2 years ago

r0mainK commented 4 years ago

Hey !

So first off thanks for the repo, it really helped having a tested CRF in pytorch. I spent the past couple cycles improving your code, and thought I would PR. Here is the list of changes I PR ed here (high level, we can talk of each commit during the review):

There is also some minor changes, like adding pytorch to the setup.py. I think the commits are well split so it should be no problem to review. I tested on my computer with pytorch 1.0, 1.1 and 1.4, all work.

PS: I tried to keep your coding style as much as possible, but I would recommend removing the line comments. Your code is good enough so that is impedes comprehension in my opinion.

EDIT: Added optimization of numerator computation

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling acae768de081a2739a0b6e7e84b921755c73344b on r0mainK:refactor-old into 1b7ced20c01352ccf70b7ae7bb99a58366de8e48 on kmkurn:master.

kmkurn commented 4 years ago

Hi, thank you for the PR! Really appreciate it. However my hands are a bit full at the moment, and this is not a small PR, so it might take a while before I can review. But from skimming the code:

Thanks again for the PR!

r0mainK commented 4 years ago

No problem, I'm not in a rush :)

I'll remove both commits, indeed I hadn't seen that the reset_parameters was idiomatic, I had checked only the superclass nn.Module but not the actual instances !