search

CogComp / saul

Saul : Declarative Learning-Based Programming
Other
64 stars 18 forks source link

Loss augmented inference using SparseNetworks #445

Closed kordjamshidi closed 7 years ago

kordjamshidi commented 8 years ago

Also added the badge example with constraints as a test example.

kordjamshidi commented 8 years ago

@bhargav tests don't fail on my machine, I am not sure why semaphore is failing.

kordjamshidi commented 8 years ago

great, passed! please feel free to review at your earliest convenience!

kordjamshidi commented 8 years ago

Yes, parameter setting is a different issue that certainly should be done in a sophisticated way for join setting as well.

kordjamshidi commented 8 years ago

seems, it is almost done.

kordjamshidi commented 8 years ago

please merge if this is ok.

kordjamshidi commented 8 years ago

done.

danyaljj commented 8 years ago

This PR looks good to me, except a few things:

  1. Semaphore looks unhappy.
  2. Could you keep the logger changes locally (not check in the PR), until we fix the logger issue? (very soon; I'll have it in my plans after #401)
  3. Regarding adding names here and there, my suggestion is to clean them all. Git will keep track of all of your contributions, with much more detail. If we want to keep them, we probably have to make it consistent (adding them to all the files).
  4. If there any way to verify that the loss-augmented inference is working? I just want prefer not check in something that we are not sure about its functional correctness.
kordjamshidi commented 8 years ago

see my inline comments.

kordjamshidi commented 8 years ago

This PR looks good to me, except a few things:

  1. Semaphore looks unhappy.

Yes, I am experimenting on this and changed the path of SRL. This is strange that the same path has been used for SRL tests and SLRApps! So, changing the path there makes the tests fail! Something to work on it. To me, the whole SRLconfigurator should be removed.

  1. Could you keep the logger changes locally (not check in the PR), until we fix the logger issue? (very soon; I'll have it in my plans after #401)

I did not expect right after my experimental changes today you decide to merge this since this was ready a few days ago :-). Ok.

  1. Regarding adding names here and there, my suggestion is to clean them all. Git will keep track of all of your contributions, with much more detail. If we want to keep them, we probably have to make it consistent (adding them to all the files).

Actually, this has been always your suggestion to remove the names and I always start mentioning that I like to keep them. I am for keeping all.

  1. If there any way to verify that the loss-augmented inference is working?

The only thing that I could do was to describe it conceptually and then test it with the Badge example, you can see the Badge example, it is one of the run options. Also, Feel free to check it algorithmically, it is a very small code.

I just want prefer not check in something that we are not sure about its functional correctness.

Me too :-).

kordjamshidi commented 7 years ago

I returned back the temporary experimental changes and logger messages. Please merge if this looks fine.

kordjamshidi commented 7 years ago

@bhargav @danyaljj

kordjamshidi commented 7 years ago

@christos-c : nobody is responding here, it would be great if you could review this and merge.

bhargav commented 7 years ago

Changes look good to me. Only concern is that using loss-augmented inference does not seem to improve the performance (82.644 vs 83.673 without) -- But I saw that you mentioned in the documentation that we are performing better on some class labels.

Btw I don't seem to have permission to merge PRs. 😕

kordjamshidi commented 7 years ago

why? because you approved it probably?

kordjamshidi commented 7 years ago

We have a lack of people. I hope @christos-c can review and merge. (@bhargav, related to the loss, it has been tested on Badge example also in this PR and seems to work fine there. For improving the results with using the loss, a simple try would be working on reweighting the losses, I can do it in a different PR, and will have that as an option to tweak with it.)

christos-c commented 7 years ago

I'm on it now, will check and merge!

kordjamshidi commented 7 years ago

@christos-c: see if this is ok now.