two changes about joint training

CogComp / saul

Saul : Declarative Learning-Based Programming

Other

64 stars 18 forks source link

two changes about joint training #296

Open danyaljj opened 8 years ago

danyaljj commented 8 years ago

merges the two joint training functions: closes #278
adds a unit test for joint training inside the ER test

fyi @kordjamshidi

bhargav commented 8 years ago

Minor comment. Also this PR would conflict with the other PR on ClassifierUtils. LGTM! Assigning to parisa for any comments.

danyaljj commented 8 years ago

@kordjamshidi could you comment on this?

kordjamshidi commented 8 years ago

it is hard to compare since they are big chucks of moving codes, but the integration of the two is ok, if 1) you have not changed the body of jointTraining loop 2) you have tested the ER results with this change. then it is ok for me to merge. One minor note: going to the case and checking the type of classifier in each iteration seems redundant though I see this happens because of the recursion style of the loop.

kordjamshidi commented 8 years ago

This is different than the results in the readme?

kordjamshidi commented 8 years ago

Are the results ok for this change? should I merge this?

danyaljj commented 8 years ago

I want to take a closer look into results. Will do it over the weekend.

bhargav commented 8 years ago

@danyaljj You might have to do this work afresh. There have been some changes in this and related files over the last few months.

danyaljj commented 8 years ago

I'll wait for #380 and then send a fresh copy.